Improve place child #4172
Merged
Improve place child #4172
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
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Results02_replace1k
duration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
03_update10th1k_x16
duration
usedJSHeapSize
07_create10k
duration
usedJSHeapSize
filter_list
duration
usedJSHeapSize
hydrate1k
duration
usedJSHeapSize
many_updates
duration
usedJSHeapSize
text_update
duration
usedJSHeapSize
todo
duration
usedJSHeapSize
|
|
Size Change: -66 B (0%) Total Size: 57.2 kB
ℹ️ View Unchanged
|
andrewiggins
commented
Oct 26, 2023
| @@ -304,10 +310,8 @@ export function toChildArray(children, out) { | |||
| } | |||
|
|
|||
| function placeChild(parentDom, newDom, oldDom) { | |||
| if (oldDom == null || oldDom.parentNode !== parentDom) { | |||
| parentDom.insertBefore(newDom, null); | |||
| } else if (newDom != oldDom || newDom.parentNode == null) { | |||
There was a problem hiding this comment.
The newDom.parentNode == null condition can also be removed cuz it would never happen, or if did would result in a DOMException.
For this condition to be hit, newDom would have to be equal to oldDom cuz of the check prior to this one. So in that scenario, if newDom.parentNode == null, then oldDom.parentNode == null. If oldDom is unmounted (since it has no parentNode), then calling parentDom.insertBefore(newDom, oldDom) would error cuz oldDom is unmounted and you can't insert a node before another node that is unmounted.
|
For some additional context, the new Fragment tests I added are actually from our Suspense test suite. The suspense tests started failing, so I pulled out a simplified repro from those tests into its own test so it was easier to debug and see what's going on. |
|
@marvinhagemeister This PR removes usages of |
andrewiggins
requested review from
developit,
marvinhagemeister and
JoviDeCroock
marvinhagemeister
approved these changes
Oct 26, 2023
JoviDeCroock
approved these changes
Oct 27, 2023
andrewiggins
force-pushed
the
improve-place-child
branch
from
e3378e0
to
137da3c
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.
NOTE: Might be easier to review by adding
?w=1to the URLWhen unmounting DOM, we need to move
oldDom/_nextDompast the unmounted DOM so the diff can continue by comparing the next VNodes to DOM nodes that are mounted. We did this in the unmount loop but we missed a couple other places where this needs to happen as well.Let's say we have the following components:
Here, Conditional will switch from a group of DOM nodes and
null. When doing this, diffChildren would hit our null placeholder code which does update oldDom past the unmounted DOM, but it doesn't update_nextDom, soAppwouldn't see this move and try to continue diffing from an unmounted DOM node.Further, even if we set
_nextDOM, diffChildren on App wouldn't see it becauseConditionalreturnednull. Since Conditional returnednull, it would returnnullfornewDom, meaning diffChildren on App would skip over all the code that was previously under aif (newDom)check, including the code that reads_nextDom. This PR also fixes that issue so even if a component returnsnull, we update the skewed index, and will read_nextDomif necessary.Also, the test "should efficiently unmount & mount components that conditionally return null with null first child" contains an additional edge case where Conditional switching to null, matches it's previous null first child. We need to ensure we detect we still need to move oldDom and update
_nextDom.Previously we attempted to handle all of these cases by checking
oldDom.parentNode !== parentDominplaceChild. However, in our entire test case, this was only occurred when oldDom was unmounted. So this update prevents unmounted oldDom from ever reachingplaceChildremoving the need for that condition entirely, dramatically simplifyingplaceChild.