Correct _nextDom pointer if it is being unmounted#2889
Merged
marvinhagemeister merged 1 commit intomasterfrom Dec 29, 2020
Merged
Correct _nextDom pointer if it is being unmounted#2889marvinhagemeister merged 1 commit intomasterfrom
_nextDom pointer if it is being unmounted#2889marvinhagemeister merged 1 commit intomasterfrom
Conversation
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Results02_replace1kduration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
03_update10th1k_x16duration
usedJSHeapSize
07_create10kduration
usedJSHeapSize
filter_listduration
usedJSHeapSize
hydrate1kduration
usedJSHeapSize
many_updatesduration
usedJSHeapSize
text_updateduration
usedJSHeapSize
|
|
Size Change: +100 B (0%) Total Size: 41.6 kB
ℹ️ View Unchanged
|
andrewiggins
commented
Dec 29, 2020
| expect(ops).to.deep.equal([]); // Component should not have updated (empty op log) | ||
| expect(scratch.innerHTML).to.equal(html); | ||
| expectDomLogToBe([ | ||
| '<div>1Hello1.insertBefore(<span>1, <span>1)', |
Member
Author
There was a problem hiding this comment.
This original dom ops were just wrong. They don't match what the test was doing.
| clearLog(); | ||
| render(<Foo condition={false} />, scratch); | ||
| expect(scratch.innerHTML).to.equal(html, 'rendering from true to false'); | ||
| expectDomLogToBe( |
Member
Author
There was a problem hiding this comment.
Since this test was written we converted all nested arrays to Fragments so in this test where DOM ops previously happened (due to nested arrays previously being flattened) there are now none since nested arrays become Fragments
marvinhagemeister
approved these changes
Dec 29, 2020
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
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.
While investigating ways to simplify and improve
placeChildI stumbled upon a bug in how we diff Fragments. If child of a Fragment is unmounted, a Fragment's_nextDompointer could point to this unmounted child. In situations where this happens, the Fragment's parent will correct this by re-appending all the children after the Fragment. While the DOM output is correct, the way we go about achieving it is sub-optimal. This change fixes the_nextDomto not point at a DOM that is going to be unmounted.For example, take the following code:
If doing a diff from the top where
<div>2gets unmounted, Fragment 1 should finish it'sdiffChildrencall with a_nextDompointer to<div>A. However, before this fix, when Fragment 1 diffs<div>1, it sets its_nextDompointer to<div>1.nextSiblingwhich is<div>2(it hasn't been unmounted yet). This fix detects that the_nextDompointer will be unmounted and resets it to the next DOM sibling, in this case<div>A.This PR fixes a long standing Fragment issue that prevented us from asserting expected DOM operations on Fragment diffing and this PR enables us to them turn on! We can now catch regressions in how we mount & move around fragment children!
Specifically, on master the test "should handle changing node type within a Component that returns a Fragment #1326" in fragments.test.js has more than necessary dom operations. The same is true for the tests I've added. All have reduced number of DOM ops.