React Native Integration #2918
React Native Integration #2918
Conversation
scottcrossen
requested review from
Feiyang1,
hiranya911 and
hsubox76
as code owners
scottcrossen
requested review from
avolkovi and
sam-gc
and removed request for
hiranya911,
Feiyang1 and
hsubox76
| type: PersistenceType = PersistenceType.LOCAL; | ||
|
|
||
| async isAvailable(): Promise<boolean> { | ||
| return true; |
There was a problem hiding this comment.
this should check if actually running in react native
There was a problem hiding this comment.
We dependency-inject this in for builds of react-native. If this file exists then it is running in react native. Would you me to make an additional check via the helper we've created?
There was a problem hiding this comment.
for non react-native builds this function will still be available through the legacy polyfill, in those cases it should return false
There was a problem hiding this comment.
So I suck at closure: Are you saying that fireauth.storage.AsyncStorage.prototype.get was part of the public API for the SDK?
| * App React Native Builds | ||
| */ | ||
| { | ||
| input: 'src/legacy_polyfill.rn.ts', |
There was a problem hiding this comment.
old SDK only had one file for everything, so we should only have one legacy polyfill that includes everything
There was a problem hiding this comment.
Haven't we established that the old SDK did things wrong? I don't think that's good justification for making one entrypoint -- especially since the configuration of the other rollup files (app, firebase) do this as well for tree-shaking purposes. If you would like me to only have one file then we are going to have to access closure to access the dependency injected libraries we need (in extended namespace firebase.INTERNAL.reactNative.AsyncStorage). Is that what you want?
There was a problem hiding this comment.
for the polyfill we need to support the old API surface which was "wrong", the new SDK API provides multiple entry points
There was a problem hiding this comment.
In the firebase polyfill/compat layer, I want to avoid recreating the legacy internal APIs(used only by auth today) unless absolutely necessary, so it would be great if you can handle the dependencies in auth itself.
So I think it makes sense to have a rn specific entry point in the auth polyfill, because there won't be a rn entry point in @firebase/app polyfill. Otherwise, non RN applications that upgrade to the latest SDK will start to include AsyncStorage code, which increase the app size.
I'm also not sure what happens if you bundle @react-native-community/async-storage in a web application. Does it work at all?
There was a problem hiding this comment.
So I tried to bundle @react-native-community/async-storage and failed because its main field points to a Flow file.
You might get it to work by compiling the Flow file first, but It clearly says to me that user shouldn't bundle @react-native-community/async-storage themselves.
There was a problem hiding this comment.
For the compat layer we will be using the old 'react-native' dependency as before. The new PR (#2947) has this.
| @@ -8,6 +8,7 @@ | |||
| "browser": "dist/index.cjs.js", | |||
| "module": "dist/index.esm.js", | |||
| "esm2017": "dist/index.esm2017.js", | |||
| "react-native": "dist/index.rn.cjs.js", | |||
There was a problem hiding this comment.
old SDK doesn't provide a react native build
There was a problem hiding this comment.
That's because the old SDK doesn't have a rollup config and doesn't support tree shaking. packages/app and packages/firebase have the react native build that the old SDK detects and abuses
|
|
||
| import AsyncStorage from '@react-native-community/async-storage'; | ||
|
|
||
| class ReactNativeLocalPersistence implements Persistence { |
There was a problem hiding this comment.
I'll add in a followup PR once we establish how we would like the new SDK to be structured (this PR is finalized). I thought we were tracking these separately on the sheet so I didn't add unit tests.
No description provided.