-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Replace deprecated "removeEventListener" syntax #2369
Conversation
Fixes #2142 |
} | ||
}; | ||
AppState.addEventListener("change", resumeListener); | ||
localAppStateListener = AppState.addEventListener("change", resumeListener); |
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.
Not sure I understand this line. Basically token (aka localAppStateListener) from adding listener, as later on passed to scope of resumeListener right to remove it?
If so Im not sure I understand what line 216 is doing.
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.
Hello, thank you for your answer. What I wanted to do:
- initialize
localAppStateListener
with theappStateListener
parameter l.216 - assign it the return of
addEventListener
l.228 - in the event listener, access
localAppStateListener
which will hold the return fromaddEventListener
I agree that initializing localAppStateListener
might be pointless. Do you want me to make this change or did you have something else in mind?
The
|
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.
Upon reading this PR, I authored #2385 which I think is a cleaner fix. There's no reason to introduce a 3rd argument, as the only purpose of the argument is to (a) determine whether or not we have already established a resumeListener, and (b) to remove the listener at the appropriate time. When the function recurses, it only needs the subscription, not the closure itself, so rather than introduce another argument, we just change what the argument is. This PR can be closed and replaced by #2385.
Merged #2385.Closing this |
This syntax has been deprecated for over a year now. This PR addresses this.