From ecddc79bcb4bf06823f1299f58daf478295429e4 Mon Sep 17 00:00:00 2001 From: Alice Pote Date: Wed, 31 Aug 2022 10:16:30 -0700 Subject: [PATCH 1/4] test(runtime): add regression tests for `updateChildren` algorithm This adds tests which ensure that children are being correctly reconciled between re-renders and, in particular, that the handling of `ref` attrs set on those children (or _their_ children) are handled correctly. This is in part adding a regression test for the fix for the issue described in #3253. --- src/runtime/test/render-vdom.spec.tsx | 88 +++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/src/runtime/test/render-vdom.spec.tsx b/src/runtime/test/render-vdom.spec.tsx index c31262d0aea..4510b436999 100644 --- a/src/runtime/test/render-vdom.spec.tsx +++ b/src/runtime/test/render-vdom.spec.tsx @@ -1097,5 +1097,93 @@ describe('render-vdom', () => { await waitForChanges(); expect(rootInstance.counter).toEqual(2); }); + + it('should not call ref cb w/ null when children are reordered', async () => { + @Component({ tag: 'cmp-a' }) + class CmpA { + counter = 0; + divRef: HTMLElement; + @Prop() state = true; + + renderA() { + return ( +
(this.divRef = el)}> + A +
+ ); + } + + renderB() { + return
B
; + } + + render() { + return this.state + ? [this.renderB(),
middle
, this.renderA()] + : [this.renderA(),
middle
, this.renderB()]; + } + } + + const { root, rootInstance, waitForChanges } = await newSpecPage({ + components: [CmpA], + html: ``, + }); + + root.state = false; + await waitForChanges(); + // We've changed the state and forced a re-render. This tests one of the + // ways in which children can be re-ordered that the `updateChildren` algo + // can handle without having `key` attrs set. + expect(rootInstance.divRef).toEqual(root.querySelector('.a')); + }); + + it('should not call ref cb w/ null when children w/ keys are reordered', async () => { + @Component({ tag: 'cmp-a' }) + class CmpA { + counter = 0; + divRef: HTMLElement; + @Prop() state = true; + + renderA() { + return ( +
(this.divRef = el)}> + A +
+ ); + } + + renderB() { + return
B
; + } + + render() { + return this.state + ? [this.renderB(), this.renderA(), this.renderB(), this.renderB()] + : [this.renderA(), this.renderB(), this.renderB()]; + } + } + + const { root, rootInstance, waitForChanges } = await newSpecPage({ + components: [CmpA], + html: ``, + }); + + root.state = false; + await waitForChanges(); + // We've changed the state and forced a re-render where the algorithm for + // reconciling children will have to use the `key` attribute to find the + // equivalent VNode on the re-render. So if that is all working correctly + // then the value of our `divRef` property should be set correctly after + // the rerender. + // + // The reordering that is conditionally done in the `render` method of the + // test component above is specifically the type of edge case that the + // parts of the `updateChildren` algorithm which _don't_ use the `key` attr + // have trouble with. + // + // This is essentially a regression test for the issue described in + // https://github.com/ionic-team/stencil/issues/3253 + expect(rootInstance.divRef).toEqual(root.querySelector('.a')); + }); }); }); From 0a9db43cca30aeb597625c347f747de48094dbd0 Mon Sep 17 00:00:00 2001 From: Alice Pote Date: Thu, 1 Sep 2022 10:47:03 -0700 Subject: [PATCH 2/4] a few tweaks to the tests --- src/runtime/test/render-vdom.spec.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/runtime/test/render-vdom.spec.tsx b/src/runtime/test/render-vdom.spec.tsx index 4510b436999..593180c1a74 100644 --- a/src/runtime/test/render-vdom.spec.tsx +++ b/src/runtime/test/render-vdom.spec.tsx @@ -1101,7 +1101,6 @@ describe('render-vdom', () => { it('should not call ref cb w/ null when children are reordered', async () => { @Component({ tag: 'cmp-a' }) class CmpA { - counter = 0; divRef: HTMLElement; @Prop() state = true; @@ -1128,7 +1127,8 @@ describe('render-vdom', () => { components: [CmpA], html: ``, }); - + // ref should be set correctly after the first render + expect(rootInstance.divRef).toEqual(root.querySelector('.a')); root.state = false; await waitForChanges(); // We've changed the state and forced a re-render. This tests one of the @@ -1140,7 +1140,6 @@ describe('render-vdom', () => { it('should not call ref cb w/ null when children w/ keys are reordered', async () => { @Component({ tag: 'cmp-a' }) class CmpA { - counter = 0; divRef: HTMLElement; @Prop() state = true; @@ -1168,6 +1167,8 @@ describe('render-vdom', () => { html: ``, }); + // ref should be set correctly after the first render + expect(rootInstance.divRef).toEqual(root.querySelector('.a')); root.state = false; await waitForChanges(); // We've changed the state and forced a re-render where the algorithm for From 2726fa1a580d546c3e1b81aae6c7cace859405e5 Mon Sep 17 00:00:00 2001 From: Alice Pote Date: Thu, 1 Sep 2022 11:00:12 -0700 Subject: [PATCH 3/4] ensure that test fails without the key --- src/runtime/test/render-vdom.spec.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/runtime/test/render-vdom.spec.tsx b/src/runtime/test/render-vdom.spec.tsx index 593180c1a74..8cd9ba1c532 100644 --- a/src/runtime/test/render-vdom.spec.tsx +++ b/src/runtime/test/render-vdom.spec.tsx @@ -1157,8 +1157,8 @@ describe('render-vdom', () => { render() { return this.state - ? [this.renderB(), this.renderA(), this.renderB(), this.renderB()] - : [this.renderA(), this.renderB(), this.renderB()]; + ? [this.renderB(), this.renderA()] + : [this.renderA(), ]; } } From 757008ebf8eeb7b72f029722392e21d0a3d51363 Mon Sep 17 00:00:00 2001 From: Alice Pote Date: Wed, 7 Sep 2022 15:49:50 -0700 Subject: [PATCH 4/4] update the tests to have a few explanatory comments --- src/runtime/test/render-vdom.spec.tsx | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/runtime/test/render-vdom.spec.tsx b/src/runtime/test/render-vdom.spec.tsx index 8cd9ba1c532..00e939e5a27 100644 --- a/src/runtime/test/render-vdom.spec.tsx +++ b/src/runtime/test/render-vdom.spec.tsx @@ -1099,6 +1099,9 @@ describe('render-vdom', () => { }); it('should not call ref cb w/ null when children are reordered', async () => { + // this test is a regression test ensuring that the algorithm for matching + // up children across rerenders works correctly when a basic transposition is + // done (the elements at the ends of the children swap places). @Component({ tag: 'cmp-a' }) class CmpA { divRef: HTMLElement; @@ -1138,6 +1141,11 @@ describe('render-vdom', () => { }); it('should not call ref cb w/ null when children w/ keys are reordered', async () => { + // this test is a regression test ensuring that the algorithm for matching + // up children across rerenders works correctly in a situation in which it + // needs to use the `key` attribute to disambiguate them. At present, if the + // `key` attribute is _not_ present in this case then this test will fail + // because without the `key` Stencil's child-identity heuristic falls over. @Component({ tag: 'cmp-a' }) class CmpA { divRef: HTMLElement; @@ -1156,9 +1164,7 @@ describe('render-vdom', () => { } render() { - return this.state - ? [this.renderB(), this.renderA()] - : [this.renderA(), ]; + return this.state ? [this.renderB(), this.renderA()] : [this.renderA()]; } }