-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(functions): turbo modules implementation #8603
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
base: main
Are you sure you want to change the base?
Conversation
@mikehardy - Android e2e tests passing for functions. iOS has hit a snag for this reason: facebook/react-native#49250 Note - from my testing, it is maps with properties that have a It seems like it is part of v0.81.0-rc.2 release therefore it isn't in stable yet. I did try and bump test app to I've had a further think on not checking in generated code, and I'm not sure I agree with the idea it should be gitignored. Any codegen changes might be worth tracking for any bugs further down the line in my opinion. I haven't fixed the code quality check CI which is failing on generated android code, I will fix it if you agree generated code is worth checking in. |
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.
updates to improve PR
...s/functions/android/src/main/java/io/invertase/firebase/functions/NativeFunctionsModule.java
Outdated
Show resolved
Hide resolved
Hey @russellwheatley - strange one!
This should not be happening. That issue does look like the symptoms you describe, however, the issue you link resulted in a PR that was first released from main in 0.79.0-rc series (if you expand the tag list you'll see it), but more importantly, it was picked into the 0.77 and 0.78 branches, so the fix was actually present in 0.78.0 (since the issue was fixed after branch cut) and then a follow-on fix was required, and it was picked into 0.78.1 - you can see those here
Our e2e app is on 0.78.2, so we have those fixes. I was able to discard those patches. So...why is this happening now, again, with TurboModules? Something really subtle must be going on. If we think something is wrong, we'll need to make a reproducer for upstream - they have a template here https://github.com/react-native-community/reproducer-react-native it is possible to take the reproducer and add native modules to it (of any kind) and run the necessary commands to generate native code or whatever - we'll need this though as it is not possible to efficiently share firebase projects related stuff with react-native upstream with all the config and key handling and such
I'll trust you on this one, you've got your sleeves rolled up and will have more taste on it right now definitely. I usually don't get value from seeing diffs in generated code, but you may be right in this case, and either way, it's a reversible decision. There's zero harm in trying so my default stance of "implementor's choice" (read as: 'let the cook cook') wins for sure. Let's see if it has value over time and anyone can re-evaluate if not ? |
@mikehardy - I've created an issue with reproducer on react native repo: facebook/react-native#52802 |
Looks like they've got a patch queued for release in 0.81 already. When I'm done traveling (still a week or so) or maybe if I catch some time while traveling I'll try to queue pick requests for older branches otherwise this will tie us to react native 0.81 as a minimum version. We probably need to separate our e2e app so macos (which lags main react native) versions aren't holding us back from testing |
@mikehardy - I think we still have a problem with null values, I checked against the latest RN release candidate and it didn't make a difference. |
Description
Related issues
Release Summary
Checklist
Android
iOS
Other
(macOS, web)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