-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix: further improvements to effect scheduling and flushing #10971
Conversation
🦋 Changeset detectedLatest commit: a7981b6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
is_flushing_effect = true; | ||
|
||
try { | ||
// When working with custom elements, the root effects might not have a root |
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 does 'the root effects might not have a root' mean?
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.
This was prior to this PR. The render effect is custom element logic has no root
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.
couple of minor suggestions but LGTM
Co-authored-by: Rich Harris <rich.harris@vercel.com>
Co-authored-by: Rich Harris <rich.harris@vercel.com>
Co-authored-by: Rich Harris <rich.harris@vercel.com>
Fixes #10946
This a follow up to 10949. Now we have true topological ordering of effects, it makes sense to try and improve certain parts of the processing of the nested effects.
Whilst looking into the bug referenced above, I noticed that we traverse into inert/destroyed effect trees, which is just pointless. I also noticed that maybe after the refactors in this PR, maybe we could just invoke render effects directly. Everything passes but one e2e test to do with custom elements. So maybe this is promising!
I also noticed that flushing of local render effects wasn't correctly setting the counter back to zero like we do in other places – which is another reported issue when using legacy effects with
beforeUpdate
, as that triggers this code path. I added a test to capture that not failing.