Comment denoted hydration #4636
Open
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
JoviDeCroock
requested review from
developit,
andrewiggins,
marvinhagemeister,
rschristian and
Copilot
📊 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
|
There was a problem hiding this comment.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/diff/children.js:133
- [nitpick] The variable name 'toResolve' is ambiguous. It should be renamed to something more descriptive like 'commentMarkersToResolve'.
let toResolve = 0;
src/diff/children.js:134
- Ensure that the new logic for handling comment nodes is covered by tests.
while ((oldDom && oldDom.nodeType == 8) || toResolve > 0) {
|
Size Change: +785 B (+1.26%) Total Size: 63.1 kB
ℹ️ View Unchanged
|
rschristian
approved these changes
Jan 29, 2025
There was a problem hiding this comment.
Sorry this took so long, wanted to find time to go back through your RFC & the RTS pull as my memory had become rusty. Looks great!
Re:comprehensive test suite, certainly would be a big benefit though I imagine it'd be a fair bit of work to cover all the various situations. Wonder how comprehensive React's suite is, might be able to repurpose some tests with less effort than writing from scratch.
marvinhagemeister
approved these changes
Jan 30, 2025
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.
Relates to #4442
Implementation from preactjs/preact-render-to-string#376
Supersedes #4444
This implements a hydration approach with the comment markers, one thing that's noteworthy is that
oldDomin ourdiffChildrenalgorithm becomes stale.We diff the wrapping element of the Suspense and non-suspenseful child, we get to an oldDom which is the first child-node of said series. When we suspend we don't move oldDom, we can try and move it inside of
diffbut this won't affect the continueddiffChildrenso we are stuck inserting that node in front of our oldDom.This has been solved in 647d138 through correctly skipping comment nodes while placing children.
I think the last part of this research would be to set up a comprehensive test suite for both
renderToStringAsyncas well asrenderToStreamso we can have fixtures for this behaviour, what do you all think?