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
Mobile: Resolves #7687: Use .remove instead of removeEventListener #7688
Mobile: Resolves #7687: Use .remove instead of removeEventListener #7688
Conversation
React Native 0.69 [deprecated removeEventListener](https://reactnative.dev/docs/0.69/dimensions#removeeventlistener) (perhaps it was deprecated before then). It seems to have been removed in React Native 0.70. This commit replaces removeEventListener with EmitterSubscription::remove.
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.
Thanks for this fix. Just a few minor changes and we can merge
packages/app-mobile/ios/Podfile.lock
Outdated
@@ -693,7 +693,7 @@ SPEC CHECKSUMS: | |||
Flipper-RSocket: d9d9ade67cbecf6ac10730304bf5607266dd2541 | |||
FlipperKit: cbdee19bdd4e7f05472a66ce290f1b729ba3cb86 | |||
fmt: ff9d55029c625d3757ed641535fd4a75fedc7ce9 | |||
glog: 04b94705f318337d7ead9e6d17c019bd9b1f6b1b | |||
glog: 5337263514dd6f09803962437687240c5dc39aa4 |
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.
Please revert the changes on this file. On my laptop too it gets randomly updated and it's not clear why, but we shouldn't commit this
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.
This should be fixed!
packages/app-mobile/root.tsx
Outdated
Linking.removeEventListener('url', this.handleOpenURL_); | ||
if (this.appStateChangeListener_) { | ||
this.appStateChangeListener_.remove(); | ||
this.urlOpenListener = null; |
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.
Should be appStateChangeListener_
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.
Thank you for catching this! It should be fixed.
Thanks for the fix! |
By the way, the commit messages is what ends up in the changelog, so it should be more user-facing. So instead of something technical like "Use .remove instead of removeEventListener", it would be better something like "Fix startup error". |
Summary
root.tsx
,AppComponent.componentWillUnmount
usesremoveEventListener
, which has been deprecated and seems to not be present in React Native 0.70. This was causing an error when reloading the app.componentWillUnmount
inAppComponent
#7687Testing plan
yarn start
yarn watch
in the mobile app directoryNote: I attempted to create an automated test that reloads the root component, but
jest
was having trouble withimport
statements in some.js
files imported/required byroot.tsx
.