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

Prevent re-initialization of a Firebase app that is already initialized #2504

Merged
merged 9 commits into from
May 14, 2021

Conversation

cjreimer
Copy link
Contributor

THIS PERHAPS SHOULD BE A RECOMMENDATION FOR ALL USERS IN THE RELEASE NOTES AS IT WILL NOT BE FIXED WITH A REDWOOD UPGRADE. This pr only addresses the CLI firebase setup.

A firebase app will throw an error if you try to reinitialize it after it is already initialized. Thus, an additional check if already initialized:

export const firebaseClient = ((config) => {
  if (!firebase.apps.length) {
    firebase.initializeApp(config)
  }
  return firebase
})(firebaseClientConfig)

is more resilient than the previous code.

export const firebaseClient = ((config) => {
  firebase.initializeApp(config)
  return firebase
})(firebaseClientConfig)

@peterp
Copy link
Contributor

peterp commented May 13, 2021

@cjreimer Oooh, where you in a situation with this code executed twice?

@cjreimer
Copy link
Contributor Author

@peterp thanks for the review!

I first saw the error in the playground-auth app. It is generated when doing a hot reload during development changes. The error is:
image
And the simple fix proposed addresses it.

I had not seen the error in my main app we're developing until now when testing. The reason is that the initialization code in my app is in App.js, which we don't touch often. If I edit App.js, the error appears in my app as well.

It appears the issue is with webpack Hot Module Replacement (HMR), which causes a reinitialization if the affected code is changed. I have not seen the issue on a simple browser reload of the app.

This does appear to be a development time issue only at this point. Would you prefer to save a line in the codebase for an infrequent error that should only be development-related? We could move this to the docs only if someone wants to add it manually.

@peterp
Copy link
Contributor

peterp commented May 13, 2021

@cjreimer Ah, that's super valid. Let's get this merged. Could you fix the failing test?

@cjreimer
Copy link
Contributor Author

Ahh ... fixed the snapshot. The windows test is still failing. I believe it's an issue with the windows test run.

Is there an easier way to get the test to retry other than to force a commit with a dummy change?

@Tobbe
Copy link
Member

Tobbe commented May 13, 2021

Is there an easier way to get the test to retry other than to force a commit with a dummy change?

Not sure if it's a permission thing or not, but I can just click this (after clicking "Details" on the failing run)

image

@cjreimer
Copy link
Contributor Author

Thanks @Tobbe. I can't see it now after the tests are passed and I don't remember seeing that button before, but all look for it next time a test fails.

@Tobbe
Copy link
Member

Tobbe commented May 13, 2021

Click "Show all checks"
image

Click "Details"
image

Click re-run (upper right corner)
image

If you can't see it, it's probably a permission thing

@cjreimer
Copy link
Contributor Author

Must be a permission thing. This is what I see (at least on a completed successful run):

image

@Tobbe
Copy link
Member

Tobbe commented May 13, 2021

Must be a permission thing.

Yeah, totally. Too bad though :( Annoying having to push dummy commits to re-trigger the runs

Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

LGTM Thanks for this fix!

@cjreimer
Copy link
Contributor Author

cjreimer commented May 14, 2021

Glad to help! I appreciate all that the core team does on this great project!

@thedavidprice
Copy link
Contributor

Merging now. Adding to checklist for v0.33 --> Code Mods recommended if using Firebase.

Merging related doc as well now as it is helpful instruction with/without this setup change.

🚀

@thedavidprice thedavidprice merged commit 7681696 into redwoodjs:main May 14, 2021
fveauvy pushed a commit to fveauvy/redwood that referenced this pull request May 15, 2021
…ed (redwoodjs#2504)

* prevent re-initialization of a Firebase app that is already initialized

* try to force re-test of tests

* try to force re-test of tests

* try to force re-test of tests

* fixed snapshot

Co-authored-by: David Price <thedavid@thedavidprice.com>
dac09 added a commit to dac09/redwood that referenced this pull request May 19, 2021
…-codegen

* 'main' of github.com:redwoodjs/redwood: (54 commits)
  Add private methods loose explicitly (redwoodjs#2554)
  Custom `useAuth` pass through for `RedwoodApolloProvider` (redwoodjs#2455)
  Prerender all routes nested in Set with prerender prop (redwoodjs#2542)
  Upgrade eslint and prettier packages including formatting fixes (redwoodjs#2540)
  contributors updates (redwoodjs#2544)
  Rename default datasource (redwoodjs#1941)
  Add default config for Component generation (redwoodjs#1814)
  build(deps): bump core-js from 3.10.1 to 3.12.1 (redwoodjs#2481)
  upgrade babel 7.14.2 with misc babel packages (redwoodjs#2541)
  build(deps): bump http-proxy-middleware from 1.1.0 to 2.0.0 (redwoodjs#2536)
  build(deps): bump pino-pretty from 4.7.1 to 4.8.0 (redwoodjs#2534)
  build(deps): bump concurrently from 6.0.2 to 6.1.0 (redwoodjs#2533)
  build(deps-dev): bump firebase-admin from 9.7.0 to 9.8.0 (redwoodjs#2522)
  build(deps): bump esbuild-loader from 2.10.0 to 2.13.0 (redwoodjs#2518)
  build(deps): bump @graphql-tools/merge from 6.2.13 to 6.2.14 (redwoodjs#2516)
  updating minor dependency versions across packages (redwoodjs#2532)
  Add JSON headers to Function generator template (redwoodjs#2457)
  fixed firebase promises so that they can be caught (redwoodjs#2503)
  Prevent re-initialization of a Firebase app that is already initialized (redwoodjs#2504)
  build(deps-dev): bump magic-sdk from 2.7.0 to 4.3.0 (redwoodjs#2463)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants