From aa707bc94acf2d607e4efa1ab99acb8b45518254 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 17 Mar 2023 13:43:35 -0700 Subject: [PATCH 1/7] Directly set textContent when only child of an element vnode is a string --- src/diff/index.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/diff/index.js b/src/diff/index.js index b6243a8bef..d87a607869 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -432,10 +432,16 @@ function diffElementNodes( diffProps(dom, newProps, oldProps, isSvg, isHydrating); // If the new vnode didn't have dangerouslySetInnerHTML, diff its children + i = newProps.children; if (newHtml) { newVNode._children = []; + } else if (typeof i === 'string') { + // TODO: Do we need to unmount oldChildren? + if (i !== oldProps.children) { + // TODO: Should we use textContent on mount and firstChild.data on update? + dom.textContent = i; + } } else { - i = newVNode.props.children; diffChildren( dom, Array.isArray(i) ? i : [i], From cf6009827f67762584e8c43a94eaf2cfff881f20 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 17 Mar 2023 13:43:56 -0700 Subject: [PATCH 2/7] Update dom op tracking tests --- test/browser/fragments.test.js | 93 ++----------------------------- test/browser/hydrate.test.js | 2 - test/browser/keys.test.js | 15 +---- test/browser/placeholders.test.js | 2 - 4 files changed, 9 insertions(+), 103 deletions(-) diff --git a/test/browser/fragments.test.js b/test/browser/fragments.test.js index e6df094d73..469a4e348e 100644 --- a/test/browser/fragments.test.js +++ b/test/browser/fragments.test.js @@ -82,10 +82,7 @@ describe('Fragment', () => { ); expect(scratch.innerHTML).to.equal('foo'); - expectDomLogToBe([ - '.appendChild(#text)', - '
.appendChild(foo)' - ]); + expectDomLogToBe(['
.appendChild(foo)']); }); it('should render multiple children via noop renderer', () => { @@ -235,7 +232,6 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(div([div(1), span(2), span(2)])); expectDomLogToBe([ - '
.appendChild(#text)', '
122.insertBefore(
1, 1)', '1.remove()' ]); @@ -357,7 +353,6 @@ describe('Fragment', () => { expect(ops).to.deep.equal([]); expect(scratch.innerHTML).to.equal('
Hello
'); expectDomLogToBe([ - '
.appendChild(#text)', '
Hello.insertBefore(
Hello,
Hello)', '
Hello.remove()' ]); @@ -368,7 +363,6 @@ describe('Fragment', () => { expect(ops).to.deep.equal([]); expect(scratch.innerHTML).to.equal('
Hello
'); expectDomLogToBe([ - '
.appendChild(#text)', // Re-append the Stateful DOM since it has been re-parented '
Hello.insertBefore(
Hello,
Hello)', '
Hello.remove()' @@ -396,7 +390,6 @@ describe('Fragment', () => { expect(ops).to.deep.equal([]); expect(scratch.innerHTML).to.equal('
Hello
'); expectDomLogToBe([ - '
.appendChild(#text)', '
Hello.insertBefore(
Hello,
Hello)', '
Hello.remove()' ]); @@ -407,7 +400,6 @@ describe('Fragment', () => { expect(ops).to.deep.equal([]); expect(scratch.innerHTML).to.equal('
Hello
'); expectDomLogToBe([ - '
.appendChild(#text)', '
Hello.insertBefore(
Hello,
Hello)', '
Hello.remove()' ]); @@ -738,9 +730,7 @@ describe('Fragment', () => { expect(ops).to.deep.equal([]); // Component should not have updated (empty op log) expect(scratch.innerHTML).to.equal(html); expectDomLogToBe([ - '.appendChild(#text)', '
1Hello2.insertBefore(1, 1)', - '
.appendChild(#text)', '
11Hello2.insertBefore(
Hello, 1)', '1.remove()', '
Hello.remove()' @@ -752,9 +742,7 @@ describe('Fragment', () => { expect(ops).to.deep.equal([]); // Component should not have updated (empty op log) expect(scratch.innerHTML).to.equal(html); expectDomLogToBe([ - '.appendChild(#text)', '
1Hello2.insertBefore(1, 1)', - '
.appendChild(#text)', '
11Hello2.insertBefore(
Hello, 1)', '1.remove()', '
Hello.remove()' @@ -1129,13 +1117,9 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(html, 'initial render of true'); expectDomLogToBe( [ - '
  • .appendChild(#text)', '
      .appendChild(
    1. 0)', - '
    2. .appendChild(#text)', '
        0.appendChild(
      1. 1)', - '
      2. .appendChild(#text)', '
          01.appendChild(
        1. 2)', - '
        2. .appendChild(#text)', '
            012.appendChild(
          1. 3)', '
            .appendChild(
              0123)' ], @@ -1177,15 +1161,10 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(htmlForTrue, 'initial render of true'); expectDomLogToBe( [ - '
            1. .appendChild(#text)', '
                .appendChild(
              1. 0)', - '
              2. .appendChild(#text)', '
                  0.appendChild(
                1. 1)', - '
                2. .appendChild(#text)', '
                    01.appendChild(
                  1. 2)', - '
                  2. .appendChild(#text)', '
                      012.appendChild(
                    1. 3)', - '
                    2. .appendChild(#text)', '
                        0123.appendChild(
                      1. 4)', '
                        .appendChild(
                          01234)' ], @@ -1211,9 +1190,7 @@ describe('Fragment', () => { ); expectDomLogToBe( [ - '
                        1. .appendChild(#text)', '
                            034.insertBefore(
                          1. 1,
                          2. 3)', - '
                          3. .appendChild(#text)', '
                              0134.insertBefore(
                            1. 2,
                            2. 3)' ], 'rendering from false to true' @@ -1330,9 +1307,7 @@ describe('Fragment', () => { ); expectDomLogToBe([ // Mount 3 & 4 - '
                            3. .appendChild(#text)', '
                                0122.appendChild(
                              1. 3)', - '
                              2. .appendChild(#text)', '
                                  01223.appendChild(
                                1. 4)', // Remove 1 & 2 (replaced with null) '
                                2. 0.remove()', @@ -1347,9 +1322,7 @@ describe('Fragment', () => { ); expectDomLogToBe([ // Insert 0 and 1 - '
                                3. .appendChild(#text)', '
                                    2234.insertBefore(
                                  1. 0,
                                  2. 2)', - '
                                  3. .appendChild(#text)', '
                                      02234.insertBefore(
                                    1. 1,
                                    2. 2)', // Remove 3 & 4 (replaced by null) '
                                    3. 3.remove()', @@ -1401,9 +1374,7 @@ describe('Fragment', () => { ); expectDomLogToBe([ // Mount 4 & 5 - '
                                    4. .appendChild(#text)', '
                                        0123.appendChild(
                                      1. 4)', - '
                                      2. .appendChild(#text)', '
                                          01234.appendChild(
                                        1. 5)', // Remove 1 & 2 (replaced with null) '
                                        2. 0.remove()', @@ -1418,9 +1389,7 @@ describe('Fragment', () => { ); expectDomLogToBe([ // Insert 0 and 1 back into the DOM - '
                                        3. .appendChild(#text)', '
                                            2345.insertBefore(
                                          1. 0,
                                          2. 2)', - '
                                          3. .appendChild(#text)', '
                                              02345.insertBefore(
                                            1. 1,
                                            2. 2)', // Remove 4 & 5 (replaced by null) '
                                            3. 4.remove()', @@ -1497,7 +1466,6 @@ describe('Fragment', () => { [ '
                                              beepHellofoo.appendChild(
                                              Hello)', '
                                              boopfooHello.appendChild(
                                              boop)', - '
                                              .appendChild(#text)', '
                                              fooHelloboop.appendChild(
                                              boop)' ], 'rendering from false to true' @@ -1671,9 +1639,7 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(htmlForFalse); expectDomLogToBe( [ - '
                                              .appendChild(#text)', '
                                              1.insertBefore(
                                              3, #text)', - '
                                              .appendChild(#text)', '
                                              31.insertBefore(
                                              4, #text)', '#text.remove()', '
                                              2.remove()' @@ -1690,7 +1656,6 @@ describe('Fragment', () => { '
                                              34.insertBefore(#text,
                                              3)', '
                                              4.remove()', '
                                              3.remove()', - '
                                              .appendChild(#text)', '
                                              1.appendChild(
                                              2)' ], 'rendering from false to true' @@ -1746,17 +1711,11 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(htmlForTrue, 'initial render of true'); expectDomLogToBe( [ - '
                                            4. .appendChild(#text)', '
                                                .appendChild(
                                              1. 0)', - '
                                              2. .appendChild(#text)', '
                                                  0.appendChild(
                                                1. 1)', - '
                                                2. .appendChild(#text)', '
                                                    01.appendChild(
                                                  1. 2)', - '
                                                  2. .appendChild(#text)', '
                                                      012.appendChild(
                                                    1. 3)', - '
                                                    2. .appendChild(#text)', '
                                                        0123.appendChild(
                                                      1. 4)', - '
                                                      2. .appendChild(#text)', '
                                                          01234.appendChild(
                                                        1. 5)', '
                                                          .appendChild(
                                                            012345)' ], @@ -1782,9 +1741,7 @@ describe('Fragment', () => { ); expectDomLogToBe( [ - '
                                                          1. .appendChild(#text)', '
                                                              0145.insertBefore(
                                                            1. 2,
                                                            2. 4)', - '
                                                            3. .appendChild(#text)', '
                                                                01245.insertBefore(
                                                              1. 3,
                                                              2. 4)' ], 'rendering from false to true' @@ -1908,11 +1865,8 @@ describe('Fragment', () => { expect(scratch.textContent).to.equal('ABC'); expectDomLogToBe([ - '
                                                                .appendChild(#text)', '
                                                                .appendChild(
                                                                A)', - '
                                                                .appendChild(#text)', '
                                                                A.appendChild(
                                                                B)', - '
                                                                .appendChild(#text)', '
                                                                AB.appendChild(
                                                                C)' ]); }); @@ -1951,7 +1905,6 @@ describe('Fragment', () => { `
                                                                A
                                                                B2
                                                                C
                                                                ` ); expectDomLogToBe([ - '
                                                                .appendChild(#text)', '
                                                                AB1C.insertBefore(
                                                                B2,
                                                                B1)', '
                                                                B1.remove()' ]); @@ -2001,9 +1954,7 @@ describe('Fragment', () => { div([div('A'), section('B3'), section('B4'), div('C')]) ); expectDomLogToBe([ - '
                                                                .appendChild(#text)', '
                                                                AB1B2C.insertBefore(
                                                                B3,
                                                                B1)', - '
                                                                .appendChild(#text)', '
                                                                AB3B1B2C.insertBefore(
                                                                B4,
                                                                B1)', '
                                                                B2.remove()', '
                                                                B1.remove()' @@ -2041,10 +1992,7 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.eql( `
                                                                A
                                                                B
                                                                C
                                                                ` ); - expectDomLogToBe([ - '
                                                                .appendChild(#text)', - '
                                                                AC.insertBefore(
                                                                B,
                                                                C)' - ]); + expectDomLogToBe(['
                                                                AC.insertBefore(
                                                                B,
                                                                C)']); }); it('should insert in-between Fragments', () => { @@ -2079,9 +2027,7 @@ describe('Fragment', () => { `
                                                                A
                                                                B1
                                                                B2
                                                                C
                                                                ` ); expectDomLogToBe([ - '
                                                                .appendChild(#text)', '
                                                                AC.insertBefore(
                                                                B1,
                                                                C)', - '
                                                                .appendChild(#text)', '
                                                                AB1C.insertBefore(
                                                                B2,
                                                                C)' ]); }); @@ -2119,10 +2065,7 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.eql( `
                                                                A
                                                                B
                                                                C
                                                                ` ); - expectDomLogToBe([ - '
                                                                .appendChild(#text)', - '
                                                                AC.insertBefore(
                                                                B,
                                                                C)' - ]); + expectDomLogToBe(['
                                                                AC.insertBefore(
                                                                B,
                                                                C)']); }); it('should insert Fragment in-between null children', () => { @@ -2164,9 +2107,7 @@ describe('Fragment', () => { `
                                                                A
                                                                B1
                                                                B2
                                                                C
                                                                ` ); expectDomLogToBe([ - '
                                                                .appendChild(#text)', '
                                                                AC.insertBefore(
                                                                B1,
                                                                C)', - '
                                                                .appendChild(#text)', '
                                                                AB1C.insertBefore(
                                                                B2,
                                                                C)' ]); }); @@ -2208,10 +2149,7 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.eql( `
                                                                A
                                                                B
                                                                C
                                                                ` ); - expectDomLogToBe([ - '
                                                                .appendChild(#text)', - '
                                                                AC.insertBefore(
                                                                B,
                                                                C)' - ]); + expectDomLogToBe(['
                                                                AC.insertBefore(
                                                                B,
                                                                C)']); }); it('should insert Fragment in-between nested null children', () => { @@ -2257,9 +2195,7 @@ describe('Fragment', () => { `
                                                                A
                                                                B1
                                                                B2
                                                                C
                                                                ` ); expectDomLogToBe([ - '
                                                                .appendChild(#text)', '
                                                                AC.insertBefore(
                                                                B1,
                                                                C)', - '
                                                                .appendChild(#text)', '
                                                                AB1C.insertBefore(
                                                                B2,
                                                                C)' ]); }); @@ -2319,7 +2255,6 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.eql(`
                                                                A2
                                                                C
                                                                `); expectDomLogToBe([ - '.appendChild(#text)', '
                                                                AC.insertBefore(A2,
                                                                A)', '
                                                                A.remove()' ]); @@ -2386,9 +2321,7 @@ describe('Fragment', () => { `
                                                                A3A4
                                                                C
                                                                ` ); expectDomLogToBe([ - '.appendChild(#text)', '
                                                                A1A2C.insertBefore(A3,
                                                                A1)', - '.appendChild(#text)', '
                                                                A3A1A2C.insertBefore(A4,
                                                                A1)', '
                                                                A2.remove()', '
                                                                A1.remove()' @@ -2453,10 +2386,7 @@ describe('Fragment', () => { div([div('A'), div('B'), div('C')]), 'updateB' ); - expectDomLogToBe([ - '
                                                                .appendChild(#text)', - '
                                                                AC.insertBefore(
                                                                B,
                                                                C)' - ]); + expectDomLogToBe(['
                                                                AC.insertBefore(
                                                                B,
                                                                C)']); clearLog(); updateA(); @@ -2467,7 +2397,6 @@ describe('Fragment', () => { 'updateA' ); expectDomLogToBe([ - '.appendChild(#text)', '
                                                                ABC.insertBefore(A2,
                                                                A)', '
                                                                A.remove()' ]); @@ -2524,9 +2453,7 @@ describe('Fragment', () => { 'updateA' ); expectDomLogToBe([ - '.appendChild(#text)', '
                                                                A1A2.insertBefore(A3,
                                                                A1)', - '.appendChild(#text)', '
                                                                A3A1A2.insertBefore(A4,
                                                                A1)', '
                                                                A2.remove()', '
                                                                A1.remove()' @@ -2540,10 +2467,7 @@ describe('Fragment', () => { [span('A3'), span('A4'), div('B')].join(''), 'updateB' ); - expectDomLogToBe([ - '
                                                                .appendChild(#text)', - '
                                                                A3A4.appendChild(
                                                                B)' - ]); + expectDomLogToBe(['
                                                                A3A4.appendChild(
                                                                B)']); }); it('should properly place conditional elements around strictly equal vnodes', () => { @@ -2598,7 +2522,6 @@ describe('Fragment', () => { rerender(); expect(scratch.innerHTML).to.equal(top); expectDomLogToBe([ - '
                                                                .appendChild(#text)', '
                                                                NavigationContentbottom panel.insertBefore(
                                                                top panel,
                                                                Navigation)', '
                                                                bottom panel.remove()' ]); @@ -2608,7 +2531,6 @@ describe('Fragment', () => { rerender(); expect(scratch.innerHTML).to.equal(bottom); expectDomLogToBe([ - '
                                                                .appendChild(#text)', '
                                                                top panelNavigationContent.appendChild(
                                                                bottom panel)', '
                                                                top panel.remove()' ]); @@ -2618,7 +2540,6 @@ describe('Fragment', () => { rerender(); expect(scratch.innerHTML).to.equal(top); expectDomLogToBe([ - '
                                                                .appendChild(#text)', '
                                                                NavigationContentbottom panel.insertBefore(
                                                                top panel,
                                                                Navigation)', '
                                                                bottom panel.remove()' ]); @@ -2746,7 +2667,6 @@ describe('Fragment', () => { div([div(1), div(4), div('A'), div('B')]) ); expectDomLogToBe([ - '
                                                                .appendChild(#text)', '
                                                                123AB.insertBefore(
                                                                4,
                                                                2)', '
                                                                2.remove()', '
                                                                3.remove()' @@ -2844,7 +2764,6 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(div([span(1), div('A'), div('B')])); expectDomLogToBe([ - '.appendChild(#text)', '
                                                                123AB.insertBefore(1,
                                                                1)', '
                                                                2.remove()', '
                                                                3.remove()', diff --git a/test/browser/hydrate.test.js b/test/browser/hydrate.test.js index 9658b6ded3..2e4a0951ef 100644 --- a/test/browser/hydrate.test.js +++ b/test/browser/hydrate.test.js @@ -165,9 +165,7 @@ describe('hydrate()', () => { expect(scratch.innerHTML).to.equal(ul([li('1'), li('2'), li('3')])); expect(getLog()).to.deep.equal([ - '
                                                              3. .appendChild(#text)', '
                                                                  1.appendChild(
                                                                • 2)', - '
                                                                • .appendChild(#text)', '
                                                                    12.appendChild(
                                                                  • 3)' ]); }); diff --git a/test/browser/keys.test.js b/test/browser/keys.test.js index bb99932cf1..78a6d0d4d2 100644 --- a/test/browser/keys.test.js +++ b/test/browser/keys.test.js @@ -168,10 +168,7 @@ describe('keys', () => { render(, scratch); expect(scratch.textContent).to.equal('abc'); - expect(getLog()).to.deep.equal([ - '
                                                                  • .appendChild(#text)', - '
                                                                      ab.appendChild(
                                                                    1. c)' - ]); + expect(getLog()).to.deep.equal(['
                                                                        ab.appendChild(
                                                                      1. c)']); }); it('should remove keyed elements from the end', () => { @@ -199,10 +196,7 @@ describe('keys', () => { render(, scratch); expect(scratch.textContent).to.equal('abc'); - expect(getLog()).to.deep.equal([ - '
                                                                      2. .appendChild(#text)', - '
                                                                          bc.insertBefore(
                                                                        1. a,
                                                                        2. b)' - ]); + expect(getLog()).to.deep.equal(['
                                                                            bc.insertBefore(
                                                                          1. a,
                                                                          2. b)']); }); it('should remove keyed elements from the beginning', () => { @@ -230,10 +224,7 @@ describe('keys', () => { render(, scratch); expect(scratch.textContent).to.equal('abc'); - expect(getLog()).to.deep.equal([ - '
                                                                          3. .appendChild(#text)', - '
                                                                              ac.insertBefore(
                                                                            1. b,
                                                                            2. c)' - ]); + expect(getLog()).to.deep.equal(['
                                                                                ac.insertBefore(
                                                                              1. b,
                                                                              2. c)']); }); it('should remove keyed children from the middle', () => { diff --git a/test/browser/placeholders.test.js b/test/browser/placeholders.test.js index 5b52ee62e4..fe1c80f1cd 100644 --- a/test/browser/placeholders.test.js +++ b/test/browser/placeholders.test.js @@ -248,9 +248,7 @@ describe('null placeholders', () => { div([div(1), div('Nullable'), div(3), div('Nullable2')]) ); expect(getLog()).to.deep.equal([ - '
                                                                                .appendChild(#text)', '
                                                                                13.appendChild(
                                                                                Nullable2)', - '
                                                                                .appendChild(#text)', '
                                                                                13Nullable2.insertBefore(
                                                                                Nullable,
                                                                                3)' ]); }); From 13cf8ef85cda484d98519eef85b16cbe38b2e989 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 17 Mar 2023 14:39:40 -0700 Subject: [PATCH 3/7] Properly unmount old children when transition to and from a single text node --- src/diff/index.js | 17 ++++++++++++++-- test/browser/fragments.test.js | 11 +++-------- test/browser/placeholders.test.js | 12 +++++++++++ test/browser/render.test.js | 33 +++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index d87a607869..df137e8b4d 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -436,12 +436,25 @@ function diffElementNodes( if (newHtml) { newVNode._children = []; } else if (typeof i === 'string') { - // TODO: Do we need to unmount oldChildren? if (i !== oldProps.children) { - // TODO: Should we use textContent on mount and firstChild.data on update? dom.textContent = i; + + // Unmount any previous children + if (oldVNode._children) { + while ((i = oldVNode._children.pop())) { + // Setting textContent on the dom element already unmounted all DOM + // nodes of the previous children + unmount(i, oldVNode, true); + } + } } } else { + // Previous render was a single text child. New children are not so let's + // unmount the previous text child + if (typeof oldProps.children === 'string') { + dom.removeChild(dom.firstChild); + } + diffChildren( dom, Array.isArray(i) ? i : [i], diff --git a/test/browser/fragments.test.js b/test/browser/fragments.test.js index 469a4e348e..506ff11841 100644 --- a/test/browser/fragments.test.js +++ b/test/browser/fragments.test.js @@ -1639,9 +1639,9 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(htmlForFalse); expectDomLogToBe( [ - '
                                                                                1.insertBefore(
                                                                                3, #text)', - '
                                                                                31.insertBefore(
                                                                                4, #text)', '#text.remove()', + '
                                                                                .appendChild(
                                                                                3)', + '
                                                                                3.appendChild(
                                                                                4)', '
                                                                                2.remove()' ], 'rendering from true to false' @@ -1652,12 +1652,7 @@ describe('Fragment', () => { expect(scratch.innerHTML).to.equal(htmlForTrue); expectDomLogToBe( - [ - '
                                                                                34.insertBefore(#text,
                                                                                3)', - '
                                                                                4.remove()', - '
                                                                                3.remove()', - '
                                                                                1.appendChild(
                                                                                2)' - ], + ['
                                                                                1.appendChild(
                                                                                2)'], 'rendering from false to true' ); }); diff --git a/test/browser/placeholders.test.js b/test/browser/placeholders.test.js index fe1c80f1cd..603102d706 100644 --- a/test/browser/placeholders.test.js +++ b/test/browser/placeholders.test.js @@ -303,4 +303,16 @@ describe('null placeholders', () => { '#text.remove()' ]); }); + + it('should properly mount and unmount text nodes with placeholders', () => { + function App({ show = false }) { + return
                                                                                {show && 'Hello'}
                                                                                ; + } + + render(, scratch); + expect(scratch.innerHTML).to.equal('
                                                                                Hello
                                                                                '); + + render(, scratch); + expect(scratch.innerHTML).to.equal('
                                                                                '); + }); }); diff --git a/test/browser/render.test.js b/test/browser/render.test.js index a8d7e00cc7..dc6f156464 100644 --- a/test/browser/render.test.js +++ b/test/browser/render.test.js @@ -1194,4 +1194,37 @@ describe('render()', () => { render(
                                                                                , scratch); expect(scratch.firstChild.tabIndex).to.equal(defaultValue); }); + + it('should correctly transition from multiple children to single text node and back', () => { + class Child extends Component { + componentDidMount() {} + componentWillUnmount() {} + render() { + return 'Child'; + } + } + + const proto = Child.prototype; + sinon.spy(Child.prototype, 'componentDidMount'); + sinon.spy(Child.prototype, 'componentWillUnmount'); + + function App({ condition = false }) { + return
                                                                                {condition ? 'Hello' : [, b]}
                                                                                ; + } + + render(, scratch); + expect(scratch.innerHTML).to.equal('
                                                                                Childb
                                                                                '); + expect(proto.componentDidMount).to.have.been.calledOnce; + expect(proto.componentWillUnmount).to.have.not.been.called; + + render(, scratch); + expect(scratch.innerHTML).to.equal('
                                                                                Hello
                                                                                '); + expect(proto.componentDidMount).to.have.been.calledOnce; + expect(proto.componentWillUnmount).to.have.been.calledOnce; + + render(, scratch); + expect(scratch.innerHTML).to.equal('
                                                                                Childb
                                                                                '); + expect(proto.componentDidMount).to.have.been.calledTwice; + expect(proto.componentWillUnmount).to.have.been.calledOnce; + }); }); From 8b9f508fcb442efa9736e38fdd51d377fb1344f7 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 17 Mar 2023 15:21:38 -0700 Subject: [PATCH 4/7] Invoke unmount lifecycles before removing DOM --- src/diff/index.js | 14 ++++++------- .../lifecycles/componentWillUnmount.test.js | 20 +++++++++++++++++++ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index df137e8b4d..715db488c0 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -431,14 +431,11 @@ function diffElementNodes( diffProps(dom, newProps, oldProps, isSvg, isHydrating); - // If the new vnode didn't have dangerouslySetInnerHTML, diff its children - i = newProps.children; + let newChildren = newProps.children; if (newHtml) { newVNode._children = []; - } else if (typeof i === 'string') { - if (i !== oldProps.children) { - dom.textContent = i; - + } else if (typeof newChildren === 'string') { + if (newChildren !== oldProps.children) { // Unmount any previous children if (oldVNode._children) { while ((i = oldVNode._children.pop())) { @@ -447,6 +444,8 @@ function diffElementNodes( unmount(i, oldVNode, true); } } + + dom.textContent = newChildren; } } else { // Previous render was a single text child. New children are not so let's @@ -455,9 +454,10 @@ function diffElementNodes( dom.removeChild(dom.firstChild); } + // If the new vnode didn't have dangerouslySetInnerHTML, diff its children diffChildren( dom, - Array.isArray(i) ? i : [i], + Array.isArray(newChildren) ? newChildren : [newChildren], newVNode, oldVNode, globalContext, diff --git a/test/browser/lifecycles/componentWillUnmount.test.js b/test/browser/lifecycles/componentWillUnmount.test.js index 5aa0a841fb..6c8fca9450 100644 --- a/test/browser/lifecycles/componentWillUnmount.test.js +++ b/test/browser/lifecycles/componentWillUnmount.test.js @@ -68,5 +68,25 @@ describe('Lifecycle methods', () => { render(, scratch); render(null, scratch); }); + + it('should only remove dom after componentWillUnmount was called with single text child', () => { + class Foo extends Component { + componentWillUnmount() { + expect(document.getElementById('foo')).to.not.equal(null); + } + + render() { + return
                                                                                ; + } + } + + render( +
                                                                                + +
                                                                                , + scratch + ); + render(
                                                                                Text
                                                                                , scratch); + }); }); }); From 2a3bba92a60818d22a3a6598a282b81c448802bc Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Mon, 20 Mar 2023 15:17:27 -0700 Subject: [PATCH 5/7] Improve comment --- src/diff/index.js | 5 +++-- test/browser/render.test.js | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index 715db488c0..a97ead7824 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -439,8 +439,9 @@ function diffElementNodes( // Unmount any previous children if (oldVNode._children) { while ((i = oldVNode._children.pop())) { - // Setting textContent on the dom element already unmounted all DOM - // nodes of the previous children + // Setting textContent on the dom element will unmount all DOM nodes + // of the previous children, so we don't need to remove DOM in this + // call to unmount unmount(i, oldVNode, true); } } diff --git a/test/browser/render.test.js b/test/browser/render.test.js index dc6f156464..1647423309 100644 --- a/test/browser/render.test.js +++ b/test/browser/render.test.js @@ -1208,8 +1208,8 @@ describe('render()', () => { sinon.spy(Child.prototype, 'componentDidMount'); sinon.spy(Child.prototype, 'componentWillUnmount'); - function App({ condition = false }) { - return
                                                                                {condition ? 'Hello' : [, b]}
                                                                                ; + function App({ textChild = false }) { + return
                                                                                {textChild ? 'Hello' : [, b]}
                                                                                ; } render(, scratch); @@ -1217,7 +1217,7 @@ describe('render()', () => { expect(proto.componentDidMount).to.have.been.calledOnce; expect(proto.componentWillUnmount).to.have.not.been.called; - render(, scratch); + render(, scratch); expect(scratch.innerHTML).to.equal('
                                                                                Hello
                                                                                '); expect(proto.componentDidMount).to.have.been.calledOnce; expect(proto.componentWillUnmount).to.have.been.calledOnce; From 4b5e75520f47cd657be3175011a6ee508f26a519 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Mon, 3 Apr 2023 13:54:20 -0700 Subject: [PATCH 6/7] Add run tracking to hydrate benchmark --- benches/src/hydrate1k.html | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/benches/src/hydrate1k.html b/benches/src/hydrate1k.html index bf43448650..7aaccda6e2 100644 --- a/benches/src/hydrate1k.html +++ b/benches/src/hydrate1k.html @@ -22,7 +22,9 @@ measureMemory, testElementText, afterFrame, - afterFrameAsync + afterFrameAsync, + markRunStart, + markRunEnd } from './util.js'; import * as framework from 'framework'; import { getComponents } from '../src/keyed-children/components.js'; @@ -101,9 +103,12 @@ testElementText(lastRowSel, '1000'); await clickRemove(hydrateRoot, `WARMUP ${i} - prehydrate`, 1000, 1000); + // Hydrate the root const store = new Store(); store.data = baseStore.data.slice(); + markRunStart(`warmup-${i}`); createRoot(hydrateRoot).hydrate(createElement(Main, { store })); + await markRunEnd(`warmup-${i}`); // Verify hydrate has correct markup and is properly hydrated testElementText(firstRowSel, '1'); @@ -111,7 +116,7 @@ await clickRemove(hydrateRoot, `WARMUP ${i} - posthydrate`, 1000, 999); } - function timedRun() { + async function timedRun() { afterFrame(function () { performance.mark('stop'); performance.measure(measureName, 'start', 'stop'); @@ -123,12 +128,13 @@ const store = new Store(); store.data = baseStore.data.slice(); + markRunStart('final'); performance.mark('start'); createRoot(hydrateRoot).hydrate(createElement(Main, { store })); + await markRunEnd('final'); } async function main() { - // const WARMUP_COUNT = 5; const WARMUP_COUNT = 5; for (let i = 0; i < WARMUP_COUNT; i++) { await warmupRun(i); @@ -136,7 +142,7 @@ await afterFrameAsync(); - timedRun(); + await timedRun(); } initializeTemplate().then(main); From 2226d4cf7b2b7ea41f552164e03283d59afe32cd Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Tue, 4 Apr 2023 13:13:59 -0700 Subject: [PATCH 7/7] Add comment demonstrating how to add custom tracing categories to benchmarks --- benches/scripts/config.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/benches/scripts/config.js b/benches/scripts/config.js index 6897a3a364..40788d9c40 100644 --- a/benches/scripts/config.js +++ b/benches/scripts/config.js @@ -1,6 +1,7 @@ import * as path from 'path'; import { deleteAsync } from 'del'; import { writeFile, stat, mkdir } from 'fs/promises'; +import { traceCategories } from 'tachometer/lib/defaults.js'; import { repoRoot, benchesRoot, toUrl } from './utils.js'; import { defaultBenchOptions } from './bench.js'; import { prepare } from './prepare.js'; @@ -189,6 +190,14 @@ export async function generateConfig(benchPath, options) { browser.trace = { logDir: traceLogDir + // categories: [ + // ...traceCategories, + // 'devtools.timeline', + // 'disabled-by-default-devtools.timeline', + // 'disabled-by-default-devtools.cpu_profiler', + // 'disabled-by-default-cpu_profiler', + // 'disabled-by-default-v8.cpu_profiler' + // ] }; }