-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Implement deferring state updates #4760
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
Open
JoviDeCroock
wants to merge
2
commits into
v11-2
Choose a base branch
from
delay-executing-state-updates-2
base: v11-2
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
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
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Resultscreate10kduration
usedJSHeapSize
filter-listduration
usedJSHeapSize
hydrate1kduration
usedJSHeapSize
many-updatesduration
usedJSHeapSize
replace1kduration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
text-updateduration
usedJSHeapSize
tododuration
usedJSHeapSize
update10th1kduration
usedJSHeapSize
|
Size Change: -6 B (-0.01%) Total Size: 46.5 kB
ℹ️ View Unchanged
|
marvinhagemeister
approved these changes
May 21, 2025
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.
LGTM
rschristian
approved these changes
May 21, 2025
Thinking about this, a case I should test for having a bug is where it's the initial render and we trigger a render that goes to a new state and then back to the initial state. We should not allow that to bail out of render 😅 |
c376d8e
to
f820118
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Copy of #4580
Note
I realized that we can make
signals
work by having a generic sCU in there, this enables us to say that we need to run through the update because of the state hooks. One concern that I still have is where hooks and state are mixed in the same component, if the state hooks end up settling but the signal changed we risk skipping an update we need. We could fix that by doing anoptions._afterRender
check in signals as well though.This puts our implementation more in line with React which could resolve a few issues as well as reduce the byte size impact of our code. Another benefit this brings would be that we stop mutating the
Component
instance and hence can't mess with other implementations like signals.What React does is when you call
reduce
is that it enqueues an update but does not execute the action yet. It first renders the component so the outer scope to the state setter can be recaptured. This way it ensures that it deals with up-to-date props/...Essentially what this comes down to is that we store every reducer invocation and only execute it as the component is rendered. This way it has access to the outer component scope, .... This needs us to add a new
SKIP_CHILDREN
bit-flag on theVNode
so we can bail out when the state settles to equal.For this to properly work we'd need to introduce
options._afterRender
which would check all of the hooks and setSKIP_CHILDREN
when all hooks are equal.To get the last test to pass we'd need to find a way to deterministically see whether this component started the diff as we should not bail if the parent instructed the change.
I think this should generally be a performance improvement as well as we aren't touching the
Component
instance anymore on every pass.There was a failing test for the NaN case, this is a top-down update which is considered a bail so it will render 25 times and bail afterwards.
For clarity, this loop is also present in React https://stackblitz.com/edit/vitejs-vite-mqaupwag?file=src%2Fmain.tsx,src%2FApp.tsx&terminal=dev