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

Firebase generator #643

Merged
merged 24 commits into from
Jun 15, 2020
Merged

Firebase generator #643

merged 24 commits into from
Jun 15, 2020

Conversation

noire-munich
Copy link
Collaborator

@noire-munich noire-munich commented Jun 2, 2020

Resolves #642

  • Added the generator for web/src/index.js
  • Proposed a fix to allow import of modules with no namespace ( use case for firebase )
  • Proposed a auth.firebase.js.template - this doesn't seem to be implemented yet and the code can either be put into the documentation or generated as well. To discuss @peterp @thedavidprice

@thedavidprice
Copy link
Contributor

Great! Add Rob as the reviewer as he's the MasterOfGenerators™

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

It looks good, I left some ideas about improving the data-structure for the imports.

Comment on lines 22 to 25
const importStatements = imports.map((imp) => {
return `import ${imp.import} from '${imp.from}'`
const importWithFrom = imp.import === null ? '' : `${imp.import} from`
return `import ${importWithFrom} '${imp.from}'`
})
Copy link
Contributor

Choose a reason for hiding this comment

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

If my suggestion about changing the imports data-structure works out, then you could remove these lines.

Copy link
Member

@cannikin cannikin left a comment

Choose a reason for hiding this comment

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

You'll need a way to write out the custom auth.firebase template instead of the single auth template we have now. You could do a check that if the provider has its own template, write it out, otherwise use the generic auth.js one we already have.

@noire-munich
Copy link
Collaborator Author

You'll need a way to write out the custom auth.firebase template instead of the single auth template we have now. You could do a check that if the provider has its own template, write it out, otherwise use the generic auth.js one we already have.

I've sent a fix, unfortunately I have issues testing it ( the template file appears when I use the rwt kit, but when I start the generate command for some reason it disappears and therefore, cannot be found ). I'll have to leave testing to one of you I'm afraid :-, this is out of my reach.

@peterp
Copy link
Contributor

peterp commented Jun 6, 2020

I start the generate command for some reason it disappears and therefore, cannot be found ). I'll have to leave testing to one of you I'm afraid :-, this is out of my reach.

I've experienced it too, I usually run yarn to install things, and then yarn rwt cp ../<path>/<to>/<repo>

Sometimes the rwt command blows up.

@peterp
Copy link
Contributor

peterp commented Jun 6, 2020

I can definitely test it though!

@noire-munich
Copy link
Collaborator Author

ah thanks @peterp that'd be nice!

Good to know it's not an isolated issue ( isolated in front of my computer screen x) ). Both the copy and copy:watch have proved to be unreliable, I really think it was an awesome way to get people to contribute so I hope it'll be fixed soon enough :). Either way, brutally setting up symlinks can do a dirty trick to make do, but I'm happy to trade it for your solution.

@peterp peterp self-assigned this Jun 8, 2020
packages/cli/src/commands/generate/auth/auth.js Outdated Show resolved Hide resolved
packages/cli/src/commands/generate/auth/auth.js Outdated Show resolved Hide resolved
Copy link
Member

@cannikin cannikin left a comment

Choose a reason for hiding this comment

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

I love all these changes, the only thing I'd ask is to keep the template with just a .template extension. Maybe make it firebase.auth.js.template or auth.firebase.js.template. We've been keeping the .js.template extension consistent among all the generators.

@noire-munich
Copy link
Collaborator Author

@cannikin no problem, makes sense :) sent the fixes for review

@cannikin
Copy link
Member

Awesome! And you've tested both the firebase generator and at least one of the others to make sure everything still runs smooth?

@noire-munich
Copy link
Collaborator Author

Awesome! And you've tested both the firebase generator and at least one of the others to make sure everything still runs smooth?

@cannikin I have tested auth0, still runs smooth ;), unfortunately I met issues while testing firebase:

  • I can run the build
  • I can copy the framework to my test project with rwt without issue
  • all files are here in ./dist
  • when I run the auth generator for firebase, it removes the firebase template from dist before anything else can happen.

@peterp offered help with testing :) I'd say the time has come :) ( or we... test on prod.... hush hush.... x) )

@cannikin
Copy link
Member

Hmmmm...that's very strange...I don't see why it would be treated any different than any other template we have, other than the fact that the filename has 4 parts instead of 3...

@noire-munich
Copy link
Collaborator Author

I don't see either. For the sake of it I tried removing '.auth' to keep 'firebase.js.template' --> changed zits, it still gets removed while using the generate command.. Can you try it on your side?

@cannikin
Copy link
Member

@noire-munich The generator worked for me!

One thing I noticed is the indentation is off in the code that's added to web/src/index.js, everything is indented one level too deep. I think this is because of the init string in the provider, it needs to be something like this:

export const config = {
  imports: [
    `import * as firebase from 'firebase/app'`,
    `import 'firebase/auth'`,
  ],
  init: `const config = {
  apiKey: process.env.FIREBASE_API_KEY,
  authDomain: process.env.FIREBASE_AUTH_DOMAIN,
  databaseURL: process.env.FIREBASE_DATABASE_URL,
  projectId: process.env.FIREBASE_PROJECT_ID,
  storageBucket: process.env.FIREBASE_STORAGE_BUCKET,
  messagingSenderId: process.env.FIREBASE_MESSAGING_SENDER_ID,
  appId: process.env.FIREBASE_APP_ID,
}

const firebaseClient = ((config) => {
  firebase.initializeApp(config)
  return firebase
})(config)`,
  authProvider: { client: 'firebaseClient', type: 'firebase' },
}

Which looks ugly here, but it generates the code correctly!

Also can you rename the config variable that's storing the firebase options? That name is a little too generic I think, this index.js could end up with more config one day and we should be explicit. firebaseConfig perhaps?

@noire-munich
Copy link
Collaborator Author

@cannikin hooray!
Ok I changed the formatting ( that's something that always bugged me when working on generators, but I understand :) ), I also renamed the variable to be a bit more specific than you suggested.
The reason is, future work for firebase might need a bit more configuration, some that could not be used to init the client.
Have you thought about how we will handle allowing for different signin methods for the same auth? I'm assuming so far we would try to do it on config, but maybe it'll be strategy based - or else.
Anyway, glad it works :), I've pushed the requested changes.

@peterp peterp added this to the next release milestone Jun 15, 2020
@peterp peterp merged commit 50bf2be into redwoodjs:master Jun 15, 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.

Firebase: Generator
5 participants