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

Regression (v10): fresh firebase installation has peerDep error on react-native? #7522

Closed
AviVahl opened this issue Aug 3, 2023 · 12 comments

Comments

@AviVahl
Copy link

AviVahl commented Aug 3, 2023

Operating System

Fedora 38

Browser Version

N/A

Firebase SDK Version

10.1.0

Firebase SDK Product:

Irrelevant

Describe your project's tooling

Irrelevant

Describe the problem

Saw this after upgrading from v9 to v10.1.0 (now that firebaseui is compatible)
Upon installation, the following peerDep warning appears:
warning "firebase > @firebase/auth > @react-native-async-storage/[email protected]" has unmet peer dependency "react-native@^0.0.0-0 || 0.60 - 0.72 || 1000.0.0".

Steps and code to reproduce issue

mkdir firebase-test
cd firebase-test
yarn init -y
yarn add firebase
@AviVahl AviVahl added new A new issue that hasn't be categoirzed as question, bug or feature request question labels Aug 3, 2023
@jbalidiong jbalidiong added needs-attention api: auth and removed new A new issue that hasn't be categoirzed as question, bug or feature request api: firestore labels Aug 3, 2023
@pragatimodi
Copy link

@hsubox76 Is this related to #7128?

@hsubox76
Copy link
Contributor

hsubox76 commented Aug 10, 2023

Yes, so see that issue for why we added @react-native-async-storage/async-storage as an auth dependency - the React Native version of Auth requires it. This warning won't pop up for React Native users because they'll have the react-native dep (a transitive peerDependency of the async-storage package) installed. Unfortunately it will pop up for non-React Native users, which is the majority.

Fortunately, it's just a warning and shouldn't block development. I do think it's a bad situation for a React-Native-related warning to impact the majority of auth users who have nothing to do with React Native, and we'll do our best to try to find a way to remove it.

I can't currently think of a way, however. We could convert the @react-native-async-storage/async-storage package from a dependency to a peerDependency in our own package.json but that would just give the same warning on install, where the unmet peerDependency is now that package. I think the only solution would be separate top-level package.jsons for the RN bundle vs the main auth bundle, but that would require creating a new NPM package (since this is an npm/yarn install warning and not a module resolution warning, which can use nested package.jsons).

@AviVahl
Copy link
Author

AviVahl commented Aug 10, 2023

npm@9 installs peerdeps by default so this actually ends up installing react-native for end users

@gavinsharp
Copy link

npm@9 installs peerdeps by default so this actually ends up installing react-native for end users

And indeed this broke various things in our monorepo when upgrading, since react-native in turn pulled in a conflicting version of react.

@hsubox76
Copy link
Contributor

I'm not sure how to solve this except for going back to the old way of requiring RN users to manually import and include @react-native-async-storage/async-storage which is technically a breaking change for anyone who's already converted to getAuth(), but since we just implemented the new way a month ago, it might not be too late to revert it. I think we may have to do this because it's not worth breaking non-RN users (the majority) for ease of use of RN users.

@AviVahl
Copy link
Author

AviVahl commented Aug 17, 2023

Make it a peerDependency and mark it as optional: https://docs.npmjs.com/cli/v9/configuring-npm/package-json#peerdependenciesmeta
This appears to not install it by default, but still allow you to define a supported semver range.
Tested with node-fetch@2, which has an optional peerDependency on "encoding".
Both npm@9 and yarn@1 did not install, or warn about it.

@hsubox76
Copy link
Contributor

Sure, but that's still going to be a "breaking change" for any RN users who are currently using getAuth(), as they'll have to manually install @react-native-async-storage/async-storage as a dependency in their own project. I will probably add a clear error message for RN users who call getAuth() or who call initializeAuth() without providing a persistence option, to tell them to install it.

Before making it a peerDependency (as opposed to removing it as a dependency altogether) I want to double check it won't cause a problem for pnpm or yarn 2/3 or any other package installers. I know pnpm can be very strict.

@hsubox76
Copy link
Contributor

I'm hesitant about making it an optional peerDependency because I've seen issue reports like this NextJS issue: vercel/next.js#7621 where users are still getting a warning on an optional peerDependency and aren't happy about it. I think we may just have to remove it altogether and rely on the warning/documentation to tell RN users to add it.

@hsubox76
Copy link
Contributor

hsubox76 commented Aug 18, 2023

I did some more investigation and found that the NextJS "encoding" issue is not related to package installers or the package.json in any way and is caused by node-fetch having a conditional require("encoding") in its code. Looks like all the major package managers support peerDependenciesMeta optional flag, so I'm adding it to the PR.

@hsubox76
Copy link
Contributor

It's been removed as a dependency in v10.3.0 and added as an optional peerDependency using peerDependenciesMeta. Let me know if you're still seeing issues related to this dependency and I'll reopen this issue.

See the release notes for how this change affects React Native users: https://firebase.google.com/support/release-notes/js#version_1030_-_august_22_2023

@KeunwooPark
Copy link

Hi. I'm coming from this release note.
I want to share that [email protected] does not solve the problem of importing getReactNativePersistence.
Here is my question in Stack Overflow. I was using [email protected] at that time and I upgraded to [email protected], but still seeing the issue. It seems like others are also facing this issue(ref).

I'm using Expo@48 and [email protected] .

@Nantris
Copy link

Nantris commented Sep 20, 2023

Just upgraded to 10.4 from 9.23 and our app crashes now due to some error in Firebase auth. Unfortunately Hermes gives such crappy error messages that that's really all I can say. We get: [TypeError: undefined is not a function]

We're using the Auth as specified here: https://firebase.google.com/support/release-notes/js#authentication

For what it's worth we use Yarn 1.x

@firebase firebase locked and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants