-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(app-check): add AppCheck implementation #5581
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
Conversation
Note there were issues with the firestore-ios-sdk-frameworks pre-compile, it's out for now
This pull request is being automatically deployed with Vercel (learn more). react-native-firebase – ./🔍 Inspect: https://vercel.com/invertase/react-native-firebase/AZM1Gk2Ahz3FrEJ7qx9PgB83seuj react-native-firebase-next – ./website_modular🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/CGNjRzLnNr7dDz31yNZvmMvob5yj [Deployment for d7e3c48 canceled] |
Codecov Report
@@ Coverage Diff @@
## master #5581 +/- ##
==========================================
- Coverage 71.17% 71.11% -0.06%
==========================================
Files 106 107 +1
Lines 4405 4420 +15
Branches 941 942 +1
==========================================
+ Hits 3135 3143 +8
- Misses 1173 1179 +6
- Partials 97 98 +1 |
c427870
to
bf4ddd9
Compare
bf4ddd9
to
408df2d
Compare
408df2d
to
cd28e54
Compare
cd28e54
to
a4a5e1d
Compare
a4a5e1d
to
fa4064d
Compare
fa4064d
to
e7e5db6
Compare
- - AppCheck | ||
- - - Usage | ||
- '/app-check/usage' | ||
- '//static.invertase.io/assets/social/firebase-logo.png' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked, there is no AppCheck official logo that I can find.
e7e5db6
to
b5840f1
Compare
547ea2b
to
0b36387
Compare
0b36387
to
fb5df80
Compare
fb5df80
to
7a23dec
Compare
e2e version was done manually and had diverged from main run script
a full install of pods (or just installing FirebaseFirestore + dependents) can take quite a while and fail the build
…acefully previously if you made an entirely new package, patch generation would fail completely until it was formally released and available for `yarn add`
7a23dec
to
2679aa3
Compare
almost all of our e2e test failures at this point are timing-related and re-running the whole batch gets past them, increasing the re-try count so the individual tests are re-run same as the whole run always is seems a better use of resources + attention
@mikehardy I think, we should use JSI for |
@nomi9995 you think we should require a native build step (that is: NDK for java platforms) and use - for the first time - C/C++ code + makefiles etc + in this module for this API call? With the side effect that since the JSI interface is not stable so we will then have a new per-react-native-version API fragility in the module? Or am I misunderstanding how JSI would be used here? This sounds like a real maintenance headache that I have personally struggled with working with the react-native-reanimated repository and the react-native releases crew - it is a large additional maintenance burden for a module That said, I'm always open to look at PRs and I do consider performance a feature. Just know that I consider maintainability a feature as well and it is my understanding that use of JSI right now is difficult to maintain |
@mikehardy yes, you are right, JSI is a real maintenance headache and made headache many times in reanimated |
I'm still open to a PR if you are strongly motivated, react-native-vision-camera uses it without quite as many ill effects, but they ship source vs pre-built binaries and that pushes build time expansion down to consumers. Honestly I think JSI will be a future direction for the module but it will be used to best effect in Firestore for the document serialization/deserialization where performance really is critical. For AppCheck, we're just waiting on the SDK to deliver results, I would guess - could be wrong but I would guess - removing JS/native bridge latency from that is an inconsequential modification |
Description
Initial implementation of AppCheck module
What works:
@react-native-firebase/app-check
package namespace to match firebase-js-sdkappCheck
javascript namespace to match firebase-js-sdksetTokenAutoRefreshEnabled
method, and parameter onactivate
getToken
in e2e testing works!What needs implementing:
FirebaseAppCheckTokenAutoRefreshEnabled
plist entry[FIRApp configure]
in AppDelegate.m. Maybe we need a startup hook?I propose that plist entry is deferred as it can be toggled already by early adopters and it's an easy follow-on PR
I prose that refresh listeners on Android are deferred indefinitely, as they don't exist on iOS so would not be cross-platform anyway.
Related issues
#5346
wix/Detox#2933
Release Summary
Each commit is separate and works on it's own with a conventional commit message and comment
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter