Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't create an AuthProvider if one exists #1251

Merged
merged 12 commits into from
Oct 8, 2020

Conversation

amorriscode
Copy link
Contributor

This PR:

Result of command:

❯ yarn rw g auth netlify
yarn run v1.22.4
$ /Users/amorriscode/dev/example-blog/node_modules/.bin/rw g auth netlify
Existing auth provider found
✨  Done in 1.90s.

Copy link
Contributor

@thedavidprice thedavidprice left a comment

Choose a reason for hiding this comment

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

@amorriscode Thanks for getting this one going! We do indeed want to check for existing and default to not overwriting. However, we also need to provide an option to override with the --force flag.

Take a look at how the setup tailwind command works here --> https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/setup/tailwind/tailwind.js

For each step in the process where a file is created or modified, there's a check if it exists. If so, it declines to run the step unless force is passed. Example here:

const configExists = fs.existsSync(
path.join(getPaths().web.base, 'tailwind.config.js')
)
if (!configExists || force) {
await execa('yarn', ['tailwindcss', 'init'])
/**
* Later, when we can tell the vscode extension where to look for the config,
* we can put it in web/config/
*/
await execa('mv', ['tailwind.config.js', 'web/'])
}

Note: you don't really need to run this check for installation of packages because Yarn will handle that by default.

Does all this make sense?

@thedavidprice
Copy link
Contributor

@amorriscode one more FYI. A next step here after this is merged could be to move the auth.js command (and provider directory) to be a part of the new Setup command. Each auth provider will be a specific setup command, e.g. yarn rw setup auth0 or yarn rw setup netlify-auth.

Take a look at how I deprecated the previous rw g util tailwind command here https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/generate/util/tailwind and moved it here https://github.com/redwoodjs/redwood/tree/main/packages/cli/src/commands/setup/tailwind

If you do this, make sure we:

  • give deprecation info in the files as well as the CLI output
  • update the CLI Docs on the website (can just create an issue)
  • create auth-specific names for each provider setup command (because in addition to setup netlify-auth we'll also eventually have setup netlify-deploy)

Having written this out, it might be a heavier lift than I initially thought. But just let me know either way. NO pressure!

@amorriscode
Copy link
Contributor Author

@thedavidprice would love some feedback on the handling of --force. The file override was already in place in the handler for the auth lib file:

{
title: 'Generating auth lib...',
task: (_ctx, task) => {
if (apiSrcDoesExist()) {
return writeFilesTask(files(provider), { overwriteExisting: force })
} else {
task.skip('api/src not found, skipping')
}
},
},

To handle --force elsewhere, I'm detecting the current AuthProvider, replacing its init code, and updating the AuthProvider component. Let me know what you think! It seems to work alright on my end.

image

As far as the other task goes (moving auth.js), I think I'll tackle that in another PR!

@amorriscode amorriscode force-pushed the handle-invalid-auth-gen branch 2 times, most recently from eb0cbd8 to 64fd4c3 Compare October 2, 2020 20:54
Copy link
Contributor

@thedavidprice thedavidprice left a comment

Choose a reason for hiding this comment

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

Anthony! This is looking great and I can tell you're thinking through all the changes and edge cases really well. Lot of work here.

In addition to the change request inline, there's one more thing that's not working correctly on --force

The section starting L225 title: 'Adding auth config to GraphQL API...', doesn't have any conditional logic in case of --force. So what happens is that you end up with a double import:

// api/src/functions/graphql.js
...
import schemas from 'src/graphql/**/*.{js,ts}'
import services from 'src/services/**/*.{js,ts}'

import { getCurrentUser } from 'src/lib/auth'

import { getCurrentUser } from 'src/lib/auth.js'
import { db } from 'src/lib/db'
...

So in case of force, you might just skip this step. A better way (more aligned with the work you've been doing here) would be to check if import { getCurrentUser } from 'src/lib/auth' exists and act accordingly.

Lastly, I did make several code change requests based on Prettier warnings. Mostly curious as to why my VS Code settings were flagging these and why they didn't just update on save on your end. Maybe it's me?

packages/cli/src/commands/generate/auth/auth.js Outdated Show resolved Hide resolved
@amorriscode
Copy link
Contributor Author

amorriscode commented Oct 3, 2020

@thedavidprice can't believe I missed the GraphQL import! 😅

As far as formatting goes, I had a TypeScript plugin that was conflicting with Prettier. Fixed my eslint plugin too so now I'll see proper warnings prior to pushing. Sorry about that!

@thedavidprice
Copy link
Contributor

All good! I'll spend some time with this on Monday in hopes of getting it merged -- but it looks great first glance. Thank you so much.

Comment on lines 146 to 147
const [_, hasAuthImport] = content.match(
/(import {.*} from 'src\/lib\/auth.*')/s
Copy link
Contributor

Choose a reason for hiding this comment

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

This is throwing an error:

object null is not iterable (cannot read property Symbol(Symbol.iterator))

But I manually checked logic for case true | false and it's working well.

Copy link
Contributor

Choose a reason for hiding this comment

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

for .match() "Matches a string or an object that supports being matched against, and returns an array containing the results of that search, or null if no matches are found."

So I believe what's happening in the case where project does not already have auth, it returns null and throws.

@thedavidprice
Copy link
Contributor

Ok, so close — just two things I found. First is the code comment about an error. Secondly, I didn't notice this before but check out these two templates files:

...and compare them to...

It turns out the first two files are missing the three comment lines, which are needed for the --force to work correctly on the file index.js. E.g. currently if you try to re-generate auth with -f and firebase, it seems you end up with the duplicate lines in web/index.js. But for netlify or auth0 it finds and replaces very nicely.

So want to confirm you're seeing the same behavior I'm seeing here. And, if so, it seems the easy resolution is to:

  • Add the three comment lines to auth/providers/firebase.js and auth/providers/magicLink.js

Sound good?

@amorriscode
Copy link
Contributor Author

@thedavidprice I'm having trouble reproducing the issue. 😢

Here is a screenshot after running yarn rw g auth magicLink --force on the example blog:
image

Here is a screenshot after running yarn rw g auth firebase --force on the example blog:
image

Two things to note:

  1. There is some extra whitespace I'm having trouble getting rid of that you can see in both screenshots
  2. I've found that I have to relink using Redwood tools after using --force; the last step of the auth gen installs packages which wipes out my link meaning if I run auth --force commands back to back, without relinking with rwt, it doesn't work

Sorry I haven't been able to resolve this on my own.

@thedavidprice
Copy link
Contributor

There is some extra whitespace I'm having trouble getting rid of that you can see in both screenshots

^^ Ah, that's low priority.

I've found that I have to relink using Redwood tools after using --force; the last step of the auth gen installs packages which wipes out my link meaning if I run auth --force commands back to back, without relinking with rwt, it doesn't work

^^ huh, that's also weird + frustrating. I've been reverting commits in my test project after each test, so maybe that's what's keeping me from having to stop, yarn --force, then yarn rwt cpw ... again.

Sorry I haven't been able to resolve this on my own.

^^ Apology not needed. I'm here to help. And you're the one doing the work! Don't stop now.

@thedavidprice
Copy link
Contributor

Ok, see my new comment inline regarding the error at line 146. Once you get setup, I think you'll hammer through this to the finish line 🏁

Here's my setup and recommendations:

  1. fresh installation: yarn create redwood-app test
  2. Git init, add, and commit
  3. do the setup dance: in Framework yarn build:watch, and in test/ project yarn rwt cpw ../dir/to-framework
  4. Fix Error: try running yarn rw g auth netlify, which should throw.
    • do magic to resolve
  5. yarn rw g auth netlify On success, commit.
  6. Open web/src/index.js
  7. Run yarn rw g auth firebase -f --> it'll have duplications in web/src/index.js, most noticeably <AuthProvider ...
  8. Revert file changes
  9. Add those three comment lines to the firebase and magicLink files
  10. Run again with -f and it works correctly with no duplications ¯_(ツ)_/¯
  11. Ship. It.

@amorriscode
Copy link
Contributor Author

Thanks for the help @thedavidprice. I still think the issue with running the command twice is that the first yarn rw g auth netlify replaces the CLI that is linked with yarn rwt cpw. I could be wrong but try this...

Test 1 (Fails)

  1. Fresh install yarn create redwood-app test
  2. Initial commit
  3. In the framework yarn build:watch then test project yarn rwt cpw ../dir/to-framework
  4. yarn rw g auth netlify (installs npm modules) --> commit
  5. yarn rw g auth firebase -f --> bad force
  6. Revert file changes

Test 2 (Succeeds)

  1. Fresh install yarn create redwood-app test
  2. Initial commit
  3. In the framework yarn build:watch then test project yarn rwt cpw ../dir/to-framework
  4. yarn rw g auth netlify (installs npm modules) --> commit
  5. Restart yarn rwt cpw ../dir/to-framwork in the test project
  6. yarn rw g auth firebase -f --> everything looks good!

@thedavidprice
Copy link
Contributor

I still think the issue with running the command twice is that the first yarn rw g auth netlify replaces the CLI that is linked with yarn rwt cpw

Gah, now my brain hurts! I could have sworn I check this the first go around and, therefore, assumed it was something with the regex matching (admittedly I didn't dig in to that part of the code). Makes sense that yarn running would bust the linking but then why... never mind. Let's get this thing merged.

Thank you and well done! 🚀

@thedavidprice thedavidprice added this to the Next release milestone Oct 8, 2020
@thedavidprice thedavidprice merged commit 0ba96d8 into redwoodjs:main Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generators: what happens when an auth generator is run more than once?
2 participants