-
-
Notifications
You must be signed in to change notification settings - Fork 835
test(runtime): add regression tests for updateChildren algorithm
#3572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
ecddc79
test(runtime): add regression tests for `updateChildren` algorithm
alicewriteswrongs 0a9db43
a few tweaks to the tests
alicewriteswrongs 2726fa1
ensure that test fails without the key
alicewriteswrongs 757008e
update the tests to have a few explanatory comments
alicewriteswrongs eb3effb
Merge branch 'main' into ap/ref-key-rerender-regression-test
rwaskiewicz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1097,5 +1097,100 @@ describe('render-vdom', () => { | |||||
| await waitForChanges(); | ||||||
| expect(rootInstance.counter).toEqual(2); | ||||||
| }); | ||||||
|
|
||||||
| 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; | ||||||
| @Prop() state = true; | ||||||
|
|
||||||
| renderA() { | ||||||
| return ( | ||||||
| <div class="a" ref={(el) => (this.divRef = el)}> | ||||||
| A | ||||||
| </div> | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| renderB() { | ||||||
| return <div>B</div>; | ||||||
| } | ||||||
|
|
||||||
| render() { | ||||||
| return this.state | ||||||
| ? [this.renderB(), <div>middle</div>, this.renderA()] | ||||||
| : [this.renderA(), <div>middle</div>, this.renderB()]; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| const { root, rootInstance, waitForChanges } = await newSpecPage({ | ||||||
| components: [CmpA], | ||||||
| html: `<cmp-a></cmp-a>`, | ||||||
| }); | ||||||
| // 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 | ||||||
| // 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 () => { | ||||||
| // 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; | ||||||
| @Prop() state = true; | ||||||
|
|
||||||
| renderA() { | ||||||
| return ( | ||||||
| <div key="a" class="a" ref={(el) => (this.divRef = el)}> | ||||||
| A | ||||||
| </div> | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| renderB() { | ||||||
| return <div>B</div>; | ||||||
| } | ||||||
|
|
||||||
| render() { | ||||||
| return this.state ? [this.renderB(), this.renderA()] : [this.renderA()]; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| const { root, rootInstance, waitForChanges } = await newSpecPage({ | ||||||
| components: [CmpA], | ||||||
| html: `<cmp-a></cmp-a>`, | ||||||
| }); | ||||||
|
|
||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense for us to put an assertion before we force the re-render that
Suggested change
That way, we know that the re-render definitely happened, as opposed to a scenario where something funky happens and the contents of |
||||||
| // 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 | ||||||
| // 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')); | ||||||
| }); | ||||||
| }); | ||||||
| }); | ||||||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I don't think I understood this the first time around..
Is the idea here that after the re-render,
divRefhasn't changed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, basically the tests are checking that in these situations the issue which showed up in Framework doesn't happen, in the one because we're doing a relatively simple reorder and the other because we're setting key attributes (on that one if you remove the
keyattr then the test should fail)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, I think where my understanding falls apart is that there's only one place we assign
this.divRef, https://github.com/ionic-team/stencil/pull/3572/files/5549f41e2500b0a7104cf6ae7c1cfa5ca821443f#diff-ec92564e32a1aaa4fbee8ec77fa6269cdfb57581ca28f2d4764c276360fa60abR1109.Should that also be on the
divreturned byrenderB, or am I not know what I don't know? 😆There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right so that
reffunction on there will be called when the related DOM node is created and when it is destroyed. So when the component first renders we call therenderAmethod and therefwill fire w/ the new DOM node as an argument. Then later on we re-render, and basically what we're testing is that when we re-render with the children in a different order theupdateChildrenalgorithm for detecting when child nodes in the new and old children are the same works correctly.Basically if that algorithm is not working correctly then what will happen is that on render no. 2 we'll trigger that
refrace condition. In that scenario, Stencil's vdom doesn't recognize that the output ofrenderAis present both in new children and old children, and so it will remove the DOM nodes associated withrenderAin the old children and create new ones for the new children. If this happens in a particular order due to the order of the child nodes in the old and new children then the removal of the old nodes will happen after the addition of the new ones, causing the ref callback to be called 3 times in totalnullas the child from render no. 1 is removedSo! Basically the test with the keys is testing out a scenario where the arrangement of the children will trigger this issue if the keys aren't present.
does that all make sense? possibly I should add some more comments to the tests describing what we're trying to test here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! That all makes sense - I do like the idea of adding a few more comments here for future folks reading these, would you please add those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwaskiewicz ok just pushed a slight update to add some comments explaining what's up