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

refactor: improve useColorScheme subscription efficiency #38001

Closed
wants to merge 3 commits into from

Conversation

vonovak
Copy link
Collaborator

@vonovak vonovak commented Jun 21, 2023

Summary:

motivation: re-rendering useColorScheme() hook with what is on the main branch means there is a subscribe + unsubscribe dance happening.

related docs: https://react.dev/reference/react/useSyncExternalStore#my-subscribe-function-gets-called-after-every-re-render

Changelog:

Test Plan:

tested locally in an app

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 21, 2023
@vonovak vonovak force-pushed the chore/useColorScheme-subs branch from a887171 to f404df5 Compare June 21, 2023 16:40
@analysis-bot
Copy link

analysis-bot commented Jun 21, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,047,627 -13,143
android hermes armeabi-v7a 8,297,052 -13,045
android hermes x86 9,563,934 -13,046
android hermes x86_64 9,406,086 -13,318
android jsc arm64-v8a 9,606,219 -7,266
android jsc armeabi-v7a 8,732,945 -7,173
android jsc x86 9,693,262 -7,170
android jsc x86_64 9,939,557 -7,442

Base commit: ebc1fe6
Branch: main

@vonovak vonovak force-pushed the chore/useColorScheme-subs branch from 924ec82 to 91ac195 Compare July 4, 2023 09:11
@facebook-github-bot
Copy link
Contributor

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

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

This pull request was successfully merged by @vonovak in b08d0df.

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

@github-actions github-actions bot added the Merged This PR has been merged. label Sep 6, 2023
@facebook-github-bot
Copy link
Contributor

@javache merged this pull request in b08d0df.

Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Sep 6, 2023
)

Summary:
motivation: re-rendering `useColorScheme()` hook with what is on the main branch means there is a subscribe + unsubscribe dance happening.

related docs: https://react.dev/reference/react/useSyncExternalStore#my-subscribe-function-gets-called-after-every-re-render

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[INTERNAL] [CHANGED] - improve useColorScheme subscription efficiency

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: facebook#38001

Test Plan: tested locally in an app

Reviewed By: rshest

Differential Revision: D49008053

Pulled By: javache

fbshipit-source-id: f8a9fc8950277e2bae8bd8774bf21312a67d46d5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants