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

Use ReactNativeAsyncStorage from community package. #7128

Merged
merged 4 commits into from
Jul 5, 2023
Merged

Conversation

hsubox76
Copy link
Contributor

@hsubox76 hsubox76 commented Mar 15, 2023

DO NOT MERGE until 6/20. This is a breaking change and must be released with a major version. It is expected to release with the next major version of the SDK in June 2023.

See internal doc: https://docs.google.com/document/d/1ZA1u7gfyIZjNpFY_v8DVBpBvDZAbxASl826OvjiTLvg/edit?resourcekey=0-AfgkOJV0qHh5waZvdLxucg#heading=h.9g9y2z36z0g9

getAuth() has been usingAsyncStorage from the React Native core package, which was deprecated for a while and has now been removed. Users are expected to get AsyncStorage from the community package (@react-native-async-storage/async-storage) now. We have tried to bridge the gap for a while by using the core AsyncStorage as the default and allowing users to import the community AsyncStorage using initializeAuth(), but the core AsyncStorage no longer exists so we can default to the community package with the next breaking change.

The community package has been added as a dependency to auth, which is an extraneous dependency for non-react-native users but it won't go into non-react-native bundles and the npm download size should be small. If the community package isn't installed, the React Native bundle will error on build, something we can't avoid as the Metro bundler doesn't allow runtime requires.

Fixes #6493
Fixes #6067

Tested this out in Expo and it's working.

@changeset-bot
Copy link

changeset-bot bot commented Mar 15, 2023

🦋 Changeset detected

Latest commit: 9a04b68

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@firebase/auth Major
firebase Major
@firebase/auth-compat Patch
@firebase/rules-unit-testing Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 15, 2023

Size Report 1

Affected Products

  • @firebase/auth

    TypeBase (60ff98d)Merge (4c32520)Diff
    react-native183 kB182 kB-842 B (-0.5%)
  • @firebase/auth/react-native

    TypeBase (60ff98d)Merge (4c32520)Diff
    browser183 kB182 kB-842 B (-0.5%)
    module183 kB182 kB-842 B (-0.5%)
  • firebase

    TypeBase (60ff98d)Merge (4c32520)Diff
    firebase-auth-react-native.js160 kB162 kB+2.75 kB (+1.7%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/kzXYMzHPPN.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 15, 2023

@hsubox76 hsubox76 changed the title Use ReactNativeAsyncStorage from community package. DO NOT MERGE: Use ReactNativeAsyncStorage from community package. Mar 15, 2023
@Neosoulink
Copy link

Thanks @hsubox76 for this PR, will help a lot when merged 🙏

@hsubox76 hsubox76 requested a review from sam-gc March 16, 2023 17:13
@@ -50,28 +49,6 @@ export {
// MFA
export { PhoneMultiFactorGenerator } from './src/platform_browser/mfa/assertions/phone';

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: This section was added as a hack to import from react-native core by default (which will normally show a warning), but not show the warning if the user overrides the default and provides the newer community version of AsyncStorage. Since this change makes the community package the default, this isn't needed anymore.

@prameshj
Copy link
Contributor

LGTM, Thanks!

Should we also update docs to call out this change + the issue with Metro and runtime deps?

@hsubox76
Copy link
Contributor Author

Note to self: ensure auth-compat also defaults to community AsyncStorage.

@@ -1969,16 +1968,6 @@ ProviderId: {
}
```

## reactNativeLocalPersistence

An implementation of [Persistence](./auth.persistence.md#persistence_interface) of type 'LOCAL' for use in React Native environments.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why this was exposed externally. Looks like the index.rn.ts was using "reactNativeLocalPersistence" as part of getAuth, but the developer wasn;t directly passing it as a param to initializeAuth, were they?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't actually remember the reason, maybe in case the users wanted to call it explicitly, or if they wanted to add multiple persistences (use the array format) for some reason, although I can't think of another persistence you'd want to add for React Native.

@prameshj
Copy link
Contributor

cc @sam-gc @renkelvin

@prameshj
Copy link
Contributor

LGTM, Thanks!

Should we also update docs to call out this change + the issue with Metro and runtime deps?

@hsubox76 bumping this back up.. should we update the web SDK docs/guide for react-native?

@hsubox76
Copy link
Contributor Author

LGTM, Thanks!
Should we also update docs to call out this change + the issue with Metro and runtime deps?

@hsubox76 bumping this back up.. should we update the web SDK docs/guide for react-native?

Thanks for the reminder, it's in blog post format right now I believe, so we can edit the blog post.

@hsubox76 hsubox76 added this to the v10 milestone May 23, 2023
keith-kurak added a commit to expo/expo that referenced this pull request Jun 5, 2023
# Why
When updating an example using the Firebase JS SDK, I found out that it
wasn't persisting the auth session, meaning you had to login again every
time you refreshed. At some point after `firebase` 9.1.3, it started
defaulting to in-memory storage**. Between all the pre-9.x instructions
still out there and the tendency for internet searches to return
instructions that work for web but not React Native, but without saying
they don't work for React Native, it's tricky to find what you need.

Further, once you find the needed `reactNativeLocalPersistence`
persistence provider, you'll find that it doesn't work in SDK 48, as it
uses the now-removed core local storage. [This at least will be fixed
soon](firebase/firebase-js-sdk#7128). While the
custom persistence code should be short-lived, at least overriding the
default persistence could be necessary indefinitely. It took me several
hours to figure this all out, the Firebase JS SDK can otherwise be a
splendid way to quickly bootstrap an app, so would like to save others
the trouble with a concise, complete, and confirmed-working code
example.

** EDIT: as I read me, it seems possible that this is instead fallback
behavior due to local persistence not working, so don't want to presume
it's an intentional default. Depending on how I apply
`reactNativeLocalPersistence`, I can get actual errors about missing
local storage functionality, or it fails silently, and the silent
failure scenario is the exact same code that used to give me a
deprecation warning, so it's unclear.

# How
Created an FYI with the details, added a callout for additional auth
configuration in the Using Firebase guide.

# Test Plan
Looked at the doc, tried the link.

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [x] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
@hsubox76 hsubox76 changed the title DO NOT MERGE: Use ReactNativeAsyncStorage from community package. Use ReactNativeAsyncStorage from community package. Jul 2, 2023
@hsubox76 hsubox76 marked this pull request as ready for review July 2, 2023 18:35
@hsubox76 hsubox76 requested review from a team and egilmorez as code owners July 2, 2023 18:35
Copy link
Contributor

@dwyfrequency dwyfrequency left a comment

Choose a reason for hiding this comment

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

LGTM

@hsubox76 hsubox76 merged commit f1c8d38 into master Jul 5, 2023
@hsubox76 hsubox76 deleted the ch-asyncstorage branch July 5, 2023 20:59
@google-oss-bot google-oss-bot mentioned this pull request Jul 6, 2023
@firebase firebase locked and limited conversation to collaborators Aug 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants