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

use-subscription tearing fix #16623

Merged
merged 3 commits into from Sep 5, 2019
Merged

Conversation

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Aug 30, 2019

Adds a test (previously failing) and a fix for a potential tearing case reported in #16396.

Resolves #16396

jest.runAllTimers();

// Partial update "C" -> "D"
// Interrupt with a second mutation "D" -> "E" but this should not cause tearing.

This comment has been minimized.

@bvaughn

bvaughn Aug 30, 2019
Author Contributor

This second interrupting mutation is required to trigger the previous failure. (Described in more detail in this issue comment.) I don't yet understand why this is but will be digging in more.

This comment has been minimized.

@bvaughn

bvaughn Aug 30, 2019
Author Contributor

It looks like React eagerly computes the state initially, for the first update. It doesn't do this or subsequent updates though because expirationTime !== NoWork.

I'm not sure if this is expected, given that my test flushes all pending work.

This comment has been minimized.

@bvaughn

bvaughn Aug 30, 2019
Author Contributor

@acdlite and I chatted about this offline and I have a better understanding of it now. I've updated the inline test comments. Going to hand this PR to him since he has full context now too.

@sizebot
Copy link

@sizebot sizebot commented Aug 30, 2019

Details of bundled changes.

Comparing: f962feb...5ba5309

use-subscription

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
use-subscription.development.js +5.0% +4.4% 6.67 KB 7 KB 2.61 KB 2.72 KB NODE_DEV
use-subscription.production.min.js -0.3% -0.2% 1.72 KB 1.71 KB 922 B 920 B NODE_PROD

Generated by 🚫 dangerJS against 5ba5309

@bvaughn bvaughn requested a review from acdlite Aug 30, 2019
@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Sep 4, 2019

Ping @acdlite :)

@acdlite
acdlite approved these changes Sep 5, 2019
@bvaughn bvaughn merged commit d96f478 into facebook:master Sep 5, 2019
13 checks passed
13 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: flow Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint_build Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test_build Your tests passed on CircleCI!
Details
ci/circleci: test_build_devtools Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: test_source Your tests passed on CircleCI!
Details
ci/circleci: test_source_persistent Your tests passed on CircleCI!
Details
ci/circleci: test_source_prod Your tests passed on CircleCI!
Details
@bvaughn bvaughn deleted the bvaughn:use-subscription-tearing branch Sep 5, 2019
This was referenced Oct 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.