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

fix(app): react-native 0.74 bridgeless mode compatibility #7688

Merged
merged 3 commits into from
May 20, 2024

Conversation

gabrieldonadel
Copy link
Contributor

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 rejection

image

Very similar case to react-native-netinfo/react-native-netinfo#717


This error happens because of the following code inside nativeModule.js

function nativeModuleWrapped(namespace, NativeModule, argToPrepend) {
const native = {};
if (!NativeModule) {
return NativeModule;
}
const properties = Object.keys(NativeModule);
for (let i = 0, len = properties.length; i < len; i++) {
const property = properties[i];
if (typeof NativeModule[property] === 'function') {
native[property] = nativeModuleMethodWrapped(namespace, NativeModule[property], argToPrepend);
} else {
native[property] = NativeModule[property];
}
}
return native;

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 on

Release Summary

Add support for react-native 0.74 bridgeless mode

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

Tested running on Android and iOS with:

  • new arch, bridgeless off
  • new arch, bridgeless on
  • old arch

🔥

Copy link

vercel bot commented Mar 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 20, 2024 0:47am

@CLAassistant
Copy link

CLAassistant commented Mar 19, 2024

CLA assistant check
All committers have signed the CLA.

@gabrieldonadel
Copy link
Contributor Author

hey @mikehardy, can you take a look at this PR when you have some time?

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Mar 25, 2024
mikehardy
mikehardy previously approved these changes Mar 25, 2024
Copy link
Collaborator

@mikehardy mikehardy left a 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

@mikehardy
Copy link
Collaborator

Something not quite right with the wrapped methods after this change when running locally, investigating...

RNFBAppCheckModule.isTokenAutoRefreshEnabled was called with 2 arguments but expects 1 arguments. If you haven't changed this method yourself, this usually means that your versions of the native code and JavaScript code are out of sync. Updating both should make this error go away.

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

Copy link

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

@github-actions github-actions bot added the Stale label Apr 22, 2024
@fobos531
Copy link

Not stale

@johnf
Copy link

johnf commented Apr 29, 2024

I tried this patch as well. My app loads but I get the following error

 (NOBRIDGE) ERROR  Error getting token [TypeError: this.native.getToken is not a function (it is undefined)]

@mikehardy
Copy link
Collaborator

mikehardy commented May 2, 2024

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

@mikehardy mikehardy added Help: Needs Triage Issue needs additional investigation/triaging. Workflow: Needs Review Pending feedback or review from a maintainer. Type: React Native Issues relating to React Native breaking changes or versions causing issues. and removed Workflow: Pending Merge Waiting on CI or similar labels May 2, 2024
@skinnynpale
Copy link

+1

@harisbaig100
Copy link

I applied it as a patch but still still (No Firebase App '[DEFAULT]') error persists

@birdofpreyru
Copy link

birdofpreyru commented May 13, 2024

Here is a that works for me on Android / New Arch / RN@0.74.1.

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];

@mikehardy
Copy link
Collaborator

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?

@mikehardy mikehardy removed the Help: Needs Triage Issue needs additional investigation/triaging. label May 20, 2024
@mikehardy mikehardy added Workflow: Waiting for User Response Blocked waiting for user response. Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Needs Review Pending feedback or review from a maintainer. labels May 20, 2024
@mikehardy
Copy link
Collaborator

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

@mikehardy
Copy link
Collaborator

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

@mikehardy mikehardy merged commit a6805bc into invertase:main May 20, 2024
14 of 15 checks passed
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label May 20, 2024
@gabrieldonadel gabrieldonadel deleted the fix-firebase-app-bridgeless branch May 20, 2024 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: React Native Issues relating to React Native breaking changes or versions causing issues. Workflow: Waiting for User Response Blocked waiting for user response.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants