-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix(app): react-native 0.74 bridgeless mode compatibility #7688
fix(app): react-native 0.74 bridgeless mode compatibility #7688
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
hey @mikehardy, can you take a look at this PR when you have some time? |
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.
🤔 interesting - looks good, sorry for the delay, my whole house caught a durable and pestilent plague a couple weeks back 😷 😆 and I've been behind ever since...all good now but oh, the backlog
Going to pull this locally and give it a run as well as seeing what CI does of course
Something not quite right with the wrapped methods after this change when running locally, investigating...
From experience, I know that the method wrapping is mostly to automatically inject the idea of which "firebase app" you are using on the native side (denoted by a string which we use to do lookups in a "firebase app name" --> "firebase app native object" map on native side The extra argument is that native firebase app name. This is a problem in the opposite direction though, where it got 2 arguments (possibly a wrapped method...) when it expected one. Anyway, I've worked through similar issues I can figure this one out I'm certain, but there might be a little delay while I do so |
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
Not stale |
I tried this patch as well. My app loads but I get the following error
|
Yeah - this is just not ready yet and I have not been able to identify exactly why yet, which is extraordinarily frustrating as the change looks perfect. In testing, it looks like the switch from making a new object vs spreading the native methods into it to using the native object directly results in our post-processing-wrap of the functions not working on the object it should be working on. Basically the wrapping fails. Why? I don't know yet |
+1 |
I applied it as a patch but still still (No Firebase App '[DEFAULT]') error persists |
Here is a that works for me on Android / New Arch / [email protected]. diff --git a/node_modules/@react-native-firebase/app/lib/internal/registry/nativeModule.js b/node_modules/@react-native-firebase/app/lib/internal/registry/nativeModule.js
index 03f001c..bd0a484 100644
--- a/node_modules/@react-native-firebase/app/lib/internal/registry/nativeModule.js
+++ b/node_modules/@react-native-firebase/app/lib/internal/registry/nativeModule.js
@@ -65,7 +65,8 @@ function nativeModuleWrapped(namespace, NativeModule, argToPrepend) {
return NativeModule;
}
- const properties = Object.keys(NativeModule);
+ let properties = Object.keys(Object.getPrototypeOf(NativeModule));
+ if (!properties.length) properties = Object.keys(NativeModule);
for (let i = 0, len = properties.length; i < len; i++) {
const property = properties[i]; |
7331ed6
to
80fcd1f
Compare
Hey @birdofpreyru that looks fantastic locally, it works on rn73 which indicates it is backwards-compatible where the previous patch wasn't. Separately I've done a lot of work cleaning up the tests so that CI can go green (it's been red on iOS in CI for a while though it was passing locally) With those changes rebased in here, the original patch reverted, and your patch added on, I think it might work Can you confirm that the final diff state of this branch (just a -1 / +2) is what you have working in your testing of new arch on rn74? |
Hmm 🤔 this works correctly for me when I apply it on top of #7774 and run it in old architecture mode but on new architecture mode my local iOS simulator says it's loading the bundle from metro then it just hangs - I think it is a local problem though - does it work for everyone here on iOS + new arch bridgeless? android new arch bridgeless works |
going to merge since it appears to be backwards compatible at least, and seems to work - if there is still a problem we can fix-forward |
Description
When trying to use
@react-native-firebase/app
with the new architecture and bridgeless mode on (react-native version 0.74.0-rc.3), the user is faced with an unhandled promise rejectionVery similar case to react-native-netinfo/react-native-netinfo#717
This error happens because of the following code inside nativeModule.js
react-native-firebase/packages/app/lib/internal/registry/nativeModule.js
Lines 62 to 79 in b40d44c
Currently we cannot copy functions directly from the React Native's NativeModules object, given that the module object is a host object.
After this change everything works as expected and the compatibility layer allows
@react-native-firebase/app
to be used with bridgeless mode onRelease Summary
Add support for react-native 0.74 bridgeless mode
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Tested running on Android and iOS with:
🔥