Skip to content

[CLI] Ethereum auth setup updates#3337

Closed
pi0neerpat wants to merge 6 commits intoredwoodjs:mainfrom
pi0neerpat:ethereum-auth-updates
Closed

[CLI] Ethereum auth setup updates#3337
pi0neerpat wants to merge 6 commits intoredwoodjs:mainfrom
pi0neerpat:ethereum-auth-updates

Conversation

@pi0neerpat
Copy link
Copy Markdown
Contributor

@pi0neerpat pi0neerpat commented Sep 2, 2021

Changes

  • Updates yarn rw setup auth ethereum to bring things up to speed with Redwood v0.36
  • Adds the template file for services/ethereumAuth/ethereumAuth.js and graphql/ethereumAuth.sdl, which removes the need to copy/paste these file from the installation instructions.

TODO

  • The build step is creating a unnecessary template file for templates/ethereum.functions.template.js. I have no idea why its getting created?!

@pi0neerpat
Copy link
Copy Markdown
Contributor Author

pi0neerpat commented Sep 3, 2021

 FAIL  src/commands/setup/auth/__tests__/addAuthConfigToApp.test.js
  ● Test suite failed to run

    TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined

      24 |     getProject().isTypeScriptProject ? 'auth.ts' : 'auth.js'
      25 |   ),
    > 26 |   service: path.join(getPaths().api.services, '/ethereumAuth/ethereumAuth.js'),
         |                 ^
      27 |   graphql: path.join(getPaths().api.graphql, 'ethereumAuth.sdl.js'),
      28 | }

The failing tests say that getPaths().api.services is undefined, but I think its rather an issue with the test environment missing this file, not related to the code in auth.js

However, it looks like this file is being mocked ??? https://github.com/redwoodjs/redwood/blob/306c26ee558d4a03bb7e234828817763dbf5b4f6/packages/cli/src/lib/test.js#L27

@thedavidprice
Copy link
Copy Markdown
Contributor

Thanks for this one @pi0neerpat I'd like to get this into the next release! Assigned "breaking" and "docs" labels — want to make sure we suggest people update their existing code per the new templates. Also, please let me know if you think the Auth docs will need updating or not.

I've looped in @dthyresson All looks good to me at a glance. I'm just not the resident expert on Auth API and templates.

@pi0neerpat
Copy link
Copy Markdown
Contributor Author

Want to make sure we suggest people update their existing code per the new templates

Agreed, thank you!

please let me know if you think the Auth docs will need updating or not.
No need for docs update on Redwood site. It still points to the repo for full instructions.

Another set of eyes would be helpful for:l

  1. Why the tests are failing
  2. Why the ethereum.functions.js is being generated, despite not having a template for it

@thedavidprice
Copy link
Copy Markdown
Contributor

@pi0neerpat I'm not seeing the file dist/**/templates/ethereum.functions.js Maybe you have an artifact that will go away if you run yarn build:clean and then yarn build? Screenshot of my dist:
Screen Shot 2021-09-16 at 4 25 20 PM

It looks like you'll need to mock the API side paths in the two failing test files (authHandler.test.js and addAuthConfigToApp.test.js). E.g:
from

jest.mock('../../../../lib', () => {
  const path = require('path')
  const __dirname = path.resolve()
  return {
    getPaths: () => ({
      api: { functions: '', src: '', lib: '' },
...

to

jest.mock('../../../../lib', () => {
  const path = require('path')
  const __dirname = path.resolve()
  return {
    getPaths: () => ({
      api: { functions: '', src: '', lib: '', services: '', graphql: '' },
...

Comment on lines +26 to +27
service: path.join(getPaths().api.services, '/ethereumAuth/ethereumAuth.js'),
graphql: path.join(getPaths().api.graphql, 'ethereumAuth.sdl.js'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this feels out of place here. Understood you'll need service and graphql path included, but hardcoding these for Ethereum Auth here probably isn't the right approach. Maybe modify files() so you can pass config from providers/etherum.js that effectively adds to the returned files? Something like this would be more extensible for any/all auth providers.

Any other (better) thoughts about this @dthyresson?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you weren't supposed to find that 🙃

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the need here is to have the setup command:

  • add SDL
  • add a service
    ?

@thedavidprice
Copy link
Copy Markdown
Contributor

@pi0neerpat just a heads up that I'm planning to create a v0.37 release branch tomorrow. No pressure, but if you have time to resolve this PR by tomorrow it would be a great one to include. And/or just communicate with me about your timing.

Thanks!

@pi0neerpat
Copy link
Copy Markdown
Contributor Author

pi0neerpat commented Sep 20, 2021 via email

@@ -0,0 +1,96 @@
import { AuthenticationError } from '@redwoodjs/api'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered instead of having this be a service with SDL, it is just a serverless function?

Does it need to be exposed to as GraphQL?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you bring up a good point. Now that I've learned how to use dbAuth, I'm thinking that I should completely re-haul the ETH Auth to piggyback on dbAuth. This is a lot, so I may need to move this to draft and come back to it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. We can move to draft and definitely review when you've had some time to rethink if need.

@dthyresson
Copy link
Copy Markdown
Contributor

Closing until reworked provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:breaking This PR is a breaking change

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants