Interaction tracking ref-counting bug fixes (WIP) #13590
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Details of bundled changes.Comparing: 7bcc077...8c1cfd5 react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
schedule
Generated by 🚫 dangerJS |
bvaughn force-pushed the interaction-ref-count-bugfix branch from 6a7a89d to 0d0788e Compare bvaughn force-pushed the interaction-ref-count-bugfix branch 3 times, most recently from f53138b to 845fccd Compare 
Contributor Author
| Okay @acdlite, I think this is ready for another look. |
bvaughn commented Sep 18, 2018
| // Do not remove the current interactions from the priority map on commit. | ||
| // This covers a special case of sync rendering with suspense. | ||
| // In this case we leave interactions in the Map until the suspended promise resolves. | ||
| let preservePendingInteractionsOnCommit: boolean = true; |
Contributor Author
There was a problem hiding this comment.
This approach causes some interactions to get lumped together in the case of a suspended+sync root with a high priority interruption, but I think that might be acceptable?
I'd be happy to discuss alternatives!
bvaughn force-pushed the interaction-ref-count-bugfix branch from 845fccd to a98e242 Compare Contributor Author
| Refactored the suspense logic based on our conversation today, and added some additional tests/fixtures @acdlite ! |
gaearon reviewed Sep 25, 2018
fixtures/tracing/test.html Outdated
| <div id="root"></div> | ||
| <!-- Load the tracing API before react to test that it's lazily evaluated --> | ||
| <script src="../../build/node_modules/schedule/umd/schedule.development.js"></script> | ||
| <script src="../../build/node_modules/schedule/umd/schedule-tracing.development.js"></script> |
Collaborator
There was a problem hiding this comment.
Needs rebase? Name changed.
Contributor Author
There was a problem hiding this comment.
Rebasing on master seems to suggest that master is broken 😄
$ yarn build BUILDING react.development.js (umd_dev) OH NOES! react.development.js (umd_dev) Error: Could not resolve 'scheduler' from /Users/bvaughn/Documents/git/react/packages/react/src/ReactSharedInternals.js at /Users/bvaughn/Documents/git/react/node_modules/rollup-plugin-node-resolve/dist/rollup-plugin-node-resolve.cjs.js:85:23 Contributor Author
There was a problem hiding this comment.
Ah, I probably just need to run yarn first huh
Contributor Author
There was a problem hiding this comment.
I always forget this silly step 😄
bvaughn force-pushed the interaction-ref-count-bugfix branch from 5662c24 to fe9446f Compare 
acdlite reviewed Sep 25, 2018
| (scheduledInteractions, scheduledExpirationTime) => { | ||
| if ( | ||
| earliestRemainingTimeAfterCommit === NoWork || | ||
| scheduledExpirationTime < earliestRemainingTimeAfterCommit |
Collaborator
There was a problem hiding this comment.
Maybe include a comment explaining why this check works, since it's a bit confusing.
acdlite approved these changes Sep 25, 2018
| // This would lead to prematurely calling the interaction-complete hook. | ||
| let suspenseDidTimeout: boolean = false; | ||
| // Instead we wait until the suspended promise has resolved. | ||
| let interactionsHaveBeenSuspended: boolean = false; |
Collaborator
There was a problem hiding this comment.
Suggestion: nextRenderIncludesTimedOutPlaceholder
Contributor Author
There was a problem hiding this comment.
This name isn't accurate for the initial null render but I don't care 😁
acdlite pushed a commit to plievone/react that referenced this pull request Oct 5, 2018
* Added new (failing) suspense+interaction tests * Add new tracing+suspense test harness fixture * Refactored interaction tracing to fix ref counting bug
Merged
This was referenced Oct 24, 2018
This was referenced Oct 24, 2018
This was referenced Nov 1, 2018
This was referenced Dec 1, 2018
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
* Added new (failing) suspense+interaction tests * Add new tracing+suspense test harness fixture * Refactored interaction tracing to fix ref counting bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Resolves #13574
Most of the changes in this diff are test code. The actual fix is in fe9446f, within the following files:
ReactFiberSchedulerReactFiberBeginWorkReactFiberUnwindWork