-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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 legacy update queue #17510
Refactor legacy update queue #17510
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 99444a2:
|
Details of bundled changes.Comparing: 3c1efa0...99444a2 react-dom
react-native-renderer
react-test-renderer
react-noop-renderer
react-art
react-reconciler
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (experimental) |
Details of bundled changes.Comparing: 3c1efa0...99444a2 react-reconciler
react-noop-renderer
react-dom
react-art
react-test-renderer
react-native-renderer
ReactDOM: size: -0.4%, gzip: 🔺+0.1% Size changes (stable) |
1cc8f85
to
275cc2f
Compare
275cc2f
to
f08b8ba
Compare
const first = pending.next; | ||
if (first !== null) { | ||
// Still circular. | ||
update.next = first; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this about, we need this for it to stay circular, no? Maybe we're missing a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All I did was remove the first !== null
check. Which is unnecessary because update.next
is never null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the diff makes it look like I deleted the assignment but really all I deleted was the type check that surrounds it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, to rephrase, it's not really a type check that I deleted. In the old implementation, the queue was only sometimes circular. It would eventually "unravel" to linear so that the old nodes could be garbage collected. But in the new implementation, the pending queue is always circular. So you don't need to check for if it's circular or not.
@@ -160,7 +160,7 @@ describe('createSubscription', () => { | |||
|
|||
it('should still work if unsubscription is managed incorrectly', async () => { | |||
const Subscription = createSubscription({ | |||
getCurrentValue: source => undefined, | |||
getCurrentValue: source => source.value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this for a while and I think the old version was just wrong and it accidentally worked with the old implementation. I'm going to try to write a failing test for what's in master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, I found the bug. Will add a test.
Adds a failing test case where an update that was committed is later skipped over during a rebase. This should never happen.
lastCapturedEffect: Update<State> | null, | ||
baseQueue: Update<State> | null, | ||
shared: SharedQueue<State>, | ||
effects: Array<Update<State>> | null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this an array as opposed to a linked list like the hooks form? Harder to see how they correlate.
I was considering a refactor to arrays but with a different take (inline fields instead of objects) . However, I wanted to do that as a separate refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk I just chose one :D
This felt like a different trade off from the hooks one because these effects are relatively rare. Only when you pass a callback to setState
. It's more like the emitEffect
API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it didn't seem worth putting a nextEffect
field on the update type
57816d9
to
862287b
Compare
This test passes in the old legacy update queue implementation. I'm adding this before the refactor to prevent a regression.
Refactors legacy update queue to incoporate rebasing fix. Uses nearly the same approach as the hook update queue but has to handle a few other cases.
862287b
to
99444a2
Compare
Fixes a bug related to rebasing updates. Once an update has committed, it should never un-commit, even if interrupted by a higher priority update. The fix includes a refactor of how update queues work. This commit is a combination of two PRs: - #17483 by @sebmarkbage refactors the hook update queue - #17510 by @acdlite refactors the class and root update queue Landing one without the other would cause state updates to sometimes be inconsistent across components, so I've combined them into a single commit in case they need to be reverted. Co-authored-by: Sebastian Markbåge <sema@fb.com> Co-authored-by: Andrew Clark <git@andrewclark.io>
I squashed this with the hooks refactor and landed as b617db3 so it can be atomically reverted. |
Fixes a bug related to rebasing updates. Once an update has committed, it should never un-commit, even if interrupted by a higher priority update. The fix includes a refactor of how update queues work. This commit is a combination of two PRs: - facebook#17483 by @sebmarkbage refactors the hook update queue - facebook#17510 by @acdlite refactors the class and root update queue Landing one without the other would cause state updates to sometimes be inconsistent across components, so I've combined them into a single commit in case they need to be reverted. Co-authored-by: Sebastian Markbåge <sema@fb.com> Co-authored-by: Andrew Clark <git@andrewclark.io>
* Refactor Update Queues to Fix Rebasing Bug Fixes a bug related to rebasing updates. Once an update has committed, it should never un-commit, even if interrupted by a higher priority update. The fix includes a refactor of how update queues work. This commit is a combination of two PRs: - #17483 by @sebmarkbage refactors the hook update queue - #17510 by @acdlite refactors the class and root update queue Landing one without the other would cause state updates to sometimes be inconsistent across components, so I've combined them into a single commit in case they need to be reverted. Co-authored-by: Sebastian Markbåge <sema@fb.com> Co-authored-by: Andrew Clark <git@andrewclark.io> * Initialize update queue object on mount Instead of lazily initializing update queue objects on the first update, class and host root queues are created on mount. This simplifies the logic for appending new updates and matches what we do for hooks.
Based on #17483
Refactors legacy update queue to incorporate rebasing fix. Uses nearly the same approach as the hook update queue but has to handle a few other cases.
Managed to remove some older, poorly factored code related to error handling.
To-do