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

(AppState) fix removeEventListener adding another listener when type is blur or focus #33491

Conversation

AntoineDoubovetzky
Copy link

Summary

I noticed the AppState.removeEventListener was in fact calling addListener instead of removeListener when type is blur or focus.

I know this method is deprecated but it can't hurt to fix it.

Changelog

[General] [Fixed] - AppState.removeEventListener correctly removes listener for blur and focus events

Test Plan

I've thought about adding a unit test, but it isn't that easy since AppState is mocked and the method is deprecated so I don't think it is worth investing too much for it.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Mar 24, 2022
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 19cf702
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,979,041 -319
android hermes armeabi-v7a 7,352,318 -332
android hermes x86 8,314,558 -322
android hermes x86_64 8,283,593 -317
android jsc arm64-v8a 9,627,056 -279
android jsc armeabi-v7a 8,401,785 -277
android jsc x86 9,576,244 -289
android jsc x86_64 10,173,220 -283

Base commit: 19cf702
Branch: main

@facebook-github-bot
Copy link
Contributor

@GijsWeterings has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @AntoineDoubovetzky in 9aab25e.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Mar 25, 2022
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…is blur or focus (facebook#33491)

Summary:
I noticed the `AppState.removeEventListener` was in fact calling `addListener` instead of `removeListener` when type is blur or focus.

I know this method is deprecated but it can't hurt to fix it.

## Changelog

[General] [Fixed] - AppState.removeEventListener correctly removes listener for blur and focus events

Pull Request resolved: facebook#33491

Test Plan: I've thought about adding a unit test, but it isn't that easy since AppState is mocked and the method is deprecated so I don't think it is worth investing too much for it.

Reviewed By: cortinico

Differential Revision: D35139808

Pulled By: GijsWeterings

fbshipit-source-id: 9d8ba157db3a62ea53759e1246f483182faf12f1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants