Raf aligned dom updates #604
Merged
Conversation
This file contains 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
🦋 Changeset detectedLatest commit: 9804e9f 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 |
✅ Deploy Preview for preact-signals-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Size Change: -25 B (-0.03%) Total Size: 83.3 kB
ℹ️ View Unchanged
|
JoviDeCroock
force-pushed
the
raf-aligned-dom-updaters
branch
8 times, most recently
from
5692de5 to
8e85bdf
Compare
JoviDeCroock
requested review from
andrewiggins,
rschristian and
marvinhagemeister
JoviDeCroock
force-pushed
the
raf-aligned-dom-updaters
branch
6 times, most recently
from
a723274 to
3c4bd59
Compare
marvinhagemeister
approved these changes
Oct 7, 2024
JoviDeCroock
commented
Oct 7, 2024
JoviDeCroock
force-pushed
the
raf-aligned-dom-updaters
branch
2 times, most recently
from
33a3591 to
dd992d4
Compare
This all works fine, however it creates a discrepancy between browser usage and test usage which we want to avoid. The discrepancy is made clear in the failing tests. What we want to happen is for Preact to be first allowed to perform its render cycle, the effects that follow and only then should we clean up stragglers by performing the direct DOM updates. Currently what happens is that the ordering is "random" based on how the signals were registered.
Co-authored-by: Ryan Christian <[email protected]>
JoviDeCroock
force-pushed
the
raf-aligned-dom-updaters
branch
6 times, most recently
from
9d43b38 to
ed0c3fd
Compare
JoviDeCroock
force-pushed
the
raf-aligned-dom-updaters
branch
from
ed0c3fd to
9804e9f
Compare
Merged
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.
This also converts our synchronous DOM updates to be animation-frame aligned, this will make them execute in parallel and when the browser is prepared for it.
Let's draw out the ideal scenario we want to create:
Now we can be sure that Preact updates take presedence over our normal DOM updates.
One thing I am unsure of is the following https://github.com/preactjs/signals/blob/main/packages/preact/src/index.ts#L92-L100 whether we can reliably make this also deferred as currently it's synchronous, we could just defer the
s.data = s.peek()part? From the tests it looks to work as we expect with RAF though 😅Fixes #315
Proof: https://codesandbox.io/p/sandbox/epic-wilbur-hhln8l
This could be considered a breaking change though 😅
Another thing to consider here would be the testing story, we could tie it into
options.debounceRenderingbut that might lead to some odd behavior in terms of rendering. To align with how the user would see this behave it would need to execute Preact updates and then update the nodes directly.In the latest version of this branch the above is fixed by leveraging
options.requestAnimationFramewhich now means that any signal update needs to be wrapped inact. In a way this was always the case as a signal update that resulted in a Preact state update would require it to be wrapped inactbut now this is the norm.Doing this investigation made me think about how we do
useSignalEffectin React and maybe we should leverage theschedulerpackage rather than injecting an arbitrary animation frame as React would better work with its own scheduler.