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

[Bugfix] Prevent already-committed setState callback from firing again during a rebase #21498

Merged
merged 2 commits into from May 12, 2021

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented May 12, 2021

First commit adds a failing test.

Second commit fixes it.

Happens during a rebase (low priority update followed by high priority
update). The high priority callback gets fired twice.
Before enqueueing the effect, adds a guard to check if the update was
already committed.
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels May 12, 2021
@acdlite acdlite requested a review from gaearon May 12, 2021 17:28
@sizebot
Copy link

sizebot commented May 12, 2021

Comparing: b770f75...a807e67

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 126.10 kB 126.12 kB +0.01% 40.46 kB 40.46 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 128.92 kB 128.94 kB +0.01% 41.39 kB 41.40 kB
facebook-www/ReactDOM-prod.classic.js = 406.36 kB 406.40 kB = 75.18 kB 75.19 kB
facebook-www/ReactDOM-prod.modern.js = 394.42 kB 394.46 kB = 73.28 kB 73.29 kB
facebook-www/ReactDOMForked-prod.classic.js = 406.36 kB 406.40 kB = 75.19 kB 75.19 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against a807e67

expect(Scheduler).toHaveYielded([0]);

await ReactNoop.act(async () => {
app.setState({step: 1}, () =>
Copy link
Member

Choose a reason for hiding this comment

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

Should this be wrapped in startTransition for the default sync case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For this test it only matters that it's lower priority than discrete/sync (default sync !== discrete sync)

@acdlite acdlite merged commit 46491dc into facebook:master May 12, 2021
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request May 25, 2021
Summary:
This sync includes the following changes:
- **[316943091](facebook/react@316943091 )**: Make StrictMode double rendering flag static for FB/www ([#21517](facebook/react#21517)) //<Brian Vaughn>//
- **[e0f89aa05](facebook/react@e0f89aa05 )**: Clean up Scheduler forks ([#20915](facebook/react#20915)) //<Ricky>//
- **[5890e0e69](facebook/react@5890e0e69 )**: Remove data-reactroot from server rendering and hydration heuristic ([#20996](facebook/react#20996)) //<Sebastian Markbåge>//
- **[46491dce9](facebook/react@46491dce9 )**: [Bugfix] Prevent already-committed setState callback from firing again during a rebase ([#21498](facebook/react#21498)) //<Andrew Clark>//
- **[b770f7500](facebook/react@b770f7500 )**: lint-build: Infer format from artifact filename ([#21489](facebook/react#21489)) //<Andrew Clark>//
- **[2bf4805e4](facebook/react@2bf4805e4 )**: Update entry point exports ([#21488](facebook/react#21488)) //<Brian Vaughn>//

Changelog:
[General][Changed] - React Native sync for revisions b8fda6c...ebcec3c

jest_e2e[run_all_tests]

Reviewed By: JoshuaGross

Differential Revision: D28572047

fbshipit-source-id: eb09c0358edb7fbf241333ea9c08724748906fea
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
…n during a rebase (facebook#21498)

* Failing test: Class callback fired multiple times

Happens during a rebase (low priority update followed by high priority
update). The high priority callback gets fired twice.

* Prevent setState callback firing during rebase

Before enqueueing the effect, adds a guard to check if the update was
already committed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants