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.

Sign up for GitHub

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

Jump to bottom

fix: forward port a bunch of deprecations / fix android hot reload #8139

Merged
merged 4 commits into from
Nov 16, 2024
Merged

fix: forward port a bunch of deprecations / fix android hot reload #8139

merged 4 commits into from
Nov 16, 2024

Conversation

mikehardy
Copy link
Collaborator

@mikehardy mikehardy commented Nov 15, 2024 •
edited
Loading

Description

I noticed while attempting to reproduce #7819 that react-native-firebase was still using onCatalystInstanceDestroy for it's module teardown, even though that no longer exists. Fixed already in Notifee and others, but not here

This implements the new invalidate hook for hot reload that should be used as of react-native 0.74 but leaves the old hook in place (delegating to this new implementation) for people still on react-native <= 0.73

Also fixed a few other deprecations I was aware of while I was in there

Related issues

Related:

Release Summary

a bunch of conventional commits ready for rebase-merge, per usual

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

run the e2e app (or an app from my make-demo.sh script) and search for "instance-destroyed" if you hot-reload it - you won't see that in adb logcat without these changes now that e2e app is on react-native 0.74+

Think react-native-firebase is great? Please consider supporting the project with any of the below:

@mikehardy
fix(android): rn74 forward-port onCatalystInstanceDestroy -> invalidate
2ea18fd 
onCatalystInstanceDestroy is no longer calld as of rn74, so our teardown
methods were not being called so we were orphaning listeners on rn 74+

this forward ports to the new invalidate API for rn74+ but leaves in place
the old hook as an override for people on rn73- and just delegates to the new
implementation
@mikehardy
fix(android): forward-port to non-deprecated data collection API
e5ec77c 
available since 2020, so should have no affect on consumers
@mikehardy
style(lint): ignore two discouraged api lints
76371d2 
one is never used apparently but shouldn't be removed in case
brownfield apps are using it as it is public static

the other is required in order to support resource load by name
functionality for remote config
@mikehardy
test(android): enable gradle deprecation warnings, fix jacoco depreca…
acad2bd 
…tion

this prepares the e2e app for gradle 9
@vercel Vercel
Copy link

vercel bot commented Nov 15, 2024 •
edited
Loading

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 Nov 15, 2024 7:16pm

@mikehardy
Copy link
Collaborator Author

I used the results of the patch-package here https://github.com/invertase/react-native-firebase/actions/runs/11862072310?pr=8139 in combo with make-demo.sh and adb logcat |grep -i auth to verify that before patches I saw "instance-initialized" but I never saw "instance-destroyed" on hot reload. With patches, I see "instance-destroyed"

et voila, we're correctly handling teardown during hot reload again

@mikehardy mikehardy merged commit efcee60 into main Nov 16, 2024
20 checks passed
@mikehardy mikehardy deleted the @mikehardy/fix-deprecations branch November 16, 2024 15:39
@mikehardy
Copy link
Collaborator Author

Turns out this actually fixes a really tricky bug, going to merge it and release it for affected users

@mikehardy mikehardy mentioned this pull request Nov 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@russellwheatley russellwheatley Awaiting requested review from russellwheatley

Assignees
No one assigned
Labels
Needs Attention platform: android
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[🐛] Android - onAuthStateChanged does not fire when I refresh the app from the metro terminal by pressing R
1 participant
@mikehardy