feat(runtime): automatically insert key attrs during compilation#5143
feat(runtime): automatically insert key attrs during compilation#5143alicewriteswrongs merged 2 commits intomainfrom
key attrs during compilation#5143Conversation
|
| Path | Error Count |
|---|---|
| src/dev-server/index.ts | 37 |
| src/mock-doc/serialize-node.ts | 36 |
| src/dev-server/server-process.ts | 32 |
| src/compiler/prerender/prerender-main.ts | 22 |
| src/testing/puppeteer/puppeteer-element.ts | 22 |
| src/runtime/client-hydrate.ts | 20 |
| src/screenshot/connector-base.ts | 19 |
| src/runtime/vdom/vdom-render.ts | 17 |
| src/compiler/config/test/validate-paths.spec.ts | 16 |
| src/dev-server/request-handler.ts | 15 |
| src/compiler/prerender/prerender-optimize.ts | 14 |
| src/compiler/sys/stencil-sys.ts | 14 |
| src/compiler/transpile/transpile-module.ts | 14 |
| src/sys/node/node-sys.ts | 14 |
| src/compiler/prerender/prerender-queue.ts | 13 |
| src/compiler/sys/in-memory-fs.ts | 13 |
| src/runtime/connected-callback.ts | 13 |
| src/runtime/set-value.ts | 13 |
| src/compiler/output-targets/output-www.ts | 12 |
| src/compiler/transformers/test/parse-vdom.spec.ts | 12 |
Our most common errors
| Typescript Error Code | Count |
|---|---|
| TS2345 | 366 |
| TS2322 | 362 |
| TS18048 | 224 |
| TS18047 | 99 |
| TS2722 | 37 |
| TS2532 | 26 |
| TS2531 | 23 |
| TS2454 | 14 |
| TS2790 | 11 |
| TS2352 | 10 |
| TS2769 | 8 |
| TS2538 | 8 |
| TS2344 | 6 |
| TS2416 | 6 |
| TS2493 | 3 |
| TS18046 | 2 |
| TS2684 | 1 |
| TS2430 | 1 |
Unused exports report
There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!
Unused exports
| File | Line | Identifier |
|---|---|---|
| src/runtime/bootstrap-lazy.ts | 21 | setNonce |
| src/screenshot/screenshot-fs.ts | 18 | readScreenshotData |
| src/testing/testing-utils.ts | 198 | withSilentWarn |
| src/utils/index.ts | 145 | CUSTOM |
| src/utils/index.ts | 269 | normalize |
| src/utils/index.ts | 7 | escapeRegExpSpecialCharacters |
| src/compiler/app-core/app-data.ts | 25 | BUILD |
| src/compiler/app-core/app-data.ts | 115 | Env |
| src/compiler/app-core/app-data.ts | 117 | NAMESPACE |
| src/compiler/fs-watch/fs-watch-rebuild.ts | 123 | updateCacheFromRebuild |
| src/compiler/types/validate-primary-package-output-target.ts | 61 | satisfies |
| src/compiler/types/validate-primary-package-output-target.ts | 61 | Record |
| src/testing/puppeteer/puppeteer-declarations.ts | 485 | WaitForEventOptions |
| src/compiler/sys/fetch/write-fetch-success.ts | 7 | writeFetchSuccessSync |
alicewriteswrongs
left a comment
There was a problem hiding this comment.
noting a few things
src/compiler/transformers/automatic-key-insertion/automatic-key-insertion.spec.ts
Outdated
Show resolved
Hide resolved
| vdomClass: false, | ||
| vdomStyle: false, | ||
| vdomKey: false, | ||
| vdomKey: true, |
There was a problem hiding this comment.
these tests for vdomKey just all changed because now every build has key attrs in it
| </div> | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
this is from a commit created by @rwaskiewicz which reproduces #1968 as a karma test.
There was a problem hiding this comment.
(ditto for the other stuff in this directory)
before adding the transformer these tests were failing, now they pass!
|
I just published |
df7b575 to
3be7b5b
Compare
This makes it possible to set a key attribute on a nested Stencil component without messing up how hydration works. This fixes a bug which was noticed while working on #5143.
ed4c36a to
9979366
Compare
c96f92b to
a690a63
Compare
051d38e to
cea23c4
Compare
src/compiler/transformers/automatic-key-insertion/automatic-key-insertion.spec.ts
Show resolved
Hide resolved
src/compiler/transformers/automatic-key-insertion/automatic-key-insertion.spec.ts
Show resolved
Hide resolved
src/compiler/transformers/automatic-key-insertion/automatic-key-insertion.spec.ts
Show resolved
Hide resolved
| expect(rect.getAttribute('transform')).toBe(null); | ||
|
|
||
| root.isOpen = true; | ||
| await waitForChanges(); | ||
| rect = root.querySelector('rect'); | ||
| expect(rect.getAttribute('transform')).toBe('rotate(45 27 27)'); | ||
|
|
||
| root.isOpen = false; | ||
| await waitForChanges(); | ||
| rect = root.querySelector('rect'); |
There was a problem hiding this comment.
For my understanding, why do we need to re-query for 'rect' now?
There was a problem hiding this comment.
a small difference in how the elements get matched up means that where before a new rect element wouldn't be created on re-render it now will be, so the reference to the rect element created on the first render isn't to the updated element created on the second re-render
There was a problem hiding this comment.
Does that apply to Stencil tests as well? Could that be considered breaking for folks that are writing tests similar to this?
There was a problem hiding this comment.
It's possible if a stencil user is writing very detailed tests that assert that within a particular component a given HTML element is either destroyed and recreated or that it's conserved across re-renders they might need to rewrite some tests. I have tested in Framework and all the tests there pass with no issues, so that's a good sign at least. Possibly we want to alert users to this? I'm not sure I would really want to classify it as a breaking change though since I think that we are fully within our rights to 'claim' the specific behavior of how Stencil's vdom produces the actual DOM that the user declaratively specifies as an implementation detail, and not itself part of the public API for building Stencil components and so on. I also haven't been able to produce counter-examples for myself which would show different user-facing behavior with this change, only 'under the hood' differences, and I think it's likely that most users wouldn't need to edit their tests with this change
There was a problem hiding this comment.
I'm not sure I agree with the premise - we may be within our right to claim behavior here, but I don't think the user would necessarily understand the distinction here. They'd just see broken tests that "worked before".
But! That all may be moot - I think in
applying this patch (revert-tests.patch, we can revert these changes safely with the latest round of commits and have the tests still pass 👍
| expect(r.children[0].getAttribute('name')).toBe('slot-b'); | ||
| expect(r.children[1].textContent.trim()).toBe('fallback default'); | ||
| expect(r.children[1].hasAttribute('hidden')).toBe(false); | ||
| expect(r.children[1].hasAttribute('hidden')).toBe(true); |
There was a problem hiding this comment.
I'm not sure I fully understand this - the implication of this code change to me is that the 'hidden' attribute's presence/value would be changing from today's existing behavior for scoped components - can you help me understand if this is the case / why we need to change this value?
There was a problem hiding this comment.
I believe what's going on is that previously this element was being destroyed and re-created so that it didn't have the hidden attribute anymore, now it is preserved across re-renders so although the value of el.getAttribute('hidden') is I believe consistent and what it's supposed to be we see a difference in whether or not the attribute has been set on the element. Does that make sense?
There was a problem hiding this comment.
I'm not sure, I'm still not totally clear as to how this is a result of automatic key insertion. Who/what was doing the recreation before, and who's doing it now? I suppose what I'm trying to understand here is any differences in our render cycle that might be a result of this change, and this question is just a small part of that.
There was a problem hiding this comment.
I realized that this is another situation where I think we want to bail before adding keys to the JSX. This is the component that's being tested in this file:
In particular, note the render() method has two returns, each of which wrap some slot stuff with <div> tags, like so:
export class SlotReorder {
@Prop() reordered = false;
render() {
if (this.reordered) {
return (
<div class="reordered">
{ ... }
</div>
);
}
return (
<div>
{ ... }
</div>
);
}
}in this case we can't safely add key attrs to those divs, because doing so will cause the runtime to not match them up across renders. In particular, our implementation of isSameVnode assumes that if keys are set on two vnodes then those are authoritative:
Note in particular that after we compare the tags on the nodes we then do this check:
if (BUILD.vdomKey && !isInitialRender) {
return leftVNode.$key$ === rightVNode.$key$;
} The transformed version of the component in question here would look something like this, with keys inserted:
export class SlotReorder {
@Prop() reordered = false;
render() {
if (this.reordered) {
return (
<div class="reordered" key="asdfasdf1234">
{ ... }
</div>
);
}
return (
<div key="hjklhjkl5678">
{ ... }
</div>
);
}
}because the keys set on those syntax nodes are not equal, when the vdom assesses whether or not these nodes are equal it will see that they have key attrs and, in that case, it will say "ok, are these equal? no? then these are not the same vnode". This will cause it to then re-render the whole thing, leading to some undesirable behavior where the <div> actually should be kept around between re-renders so that its children can be diffed as well, rather than always being re-rendered on each render cycle.
I thought about this a bit, and I think that there's just not really a safe way that we can add keys in a case like this where there are multiple returns from the component's render() method. Or, at the least, I don't think we could safely do so without adding some more fancy static analysis that I dont think we want to take on. Like adding keys would be safe here I think:
export class ImageOrText {
@Prop() showImage = false;
render() {
if (this.showImage) {
return (
<img src="..." />
);
}
return (
<span>text instead!</span>
);
}
}because the two tags are different the vdom would already have to re-render the elements when this.showImage changes, so we wouldn't run into problems adding keys to the img and span tags in the JSX. But! I don't think we probably want to bite off trying to figure out when it's safe to do this. So instead I think we need to not add keys when:
- there are two returns in a component's
rendermethod and - a component's
rendermethod returns a ternary
Does that all make sense? I've added some further constraints to when we transform JSX nodes and it makes this test in pass without the changes I have in this file right now.
|
Ahh - two more things:
|
key attrs during compilation
key attrs during compilationkey attrs during compilation
565798a to
d6e9eac
Compare
alicewriteswrongs
left a comment
There was a problem hiding this comment.
just noting a few things
| it('should not add a static key to dynamic elements', async () => { | ||
| jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValueOnce('once-key'); | ||
| const t = transpile(` | ||
| @Component({tag: 'cmp-a'}) | ||
| export class CmpA { | ||
| render() { | ||
| return ( | ||
| <div> | ||
| {this.todos.map((todo) => ( | ||
| <div>{ todo }</div> | ||
| ))} | ||
| </div> | ||
| ) | ||
| } | ||
| }`); | ||
| expect(await formatCode(t.outputText)).toBe( | ||
| `export class CmpA { | ||
| render() { | ||
| return h( | ||
| 'div', | ||
| { key: 'once-key' }, | ||
| this.todos.map((todo) => h('div', null, todo)), | ||
| ); | ||
| } | ||
| static get is() { | ||
| return 'cmp-a'; | ||
| } | ||
| } | ||
| `, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
this is a test to ensure that we don't add static keys to dynamic generated elements. I did a bit of a conservative cut-off for these - we don't attempt to transform any JSX nodes which are found within a JSX expression, so with (contrived) code like
@Component({ tag: 'cmp-a' })
class CmpA {
render() {
return <div>{ (() => <div>inner</div>)() }</div>
}
}we won't transform the 'inner' div in the IIFE because, since it will be generated at runtime, we could run into a lot of cases where we can't really know too much about what's going on there and when it would not be safe to add keys.
| const tagName = getComponentTagName(node.members.filter(isStaticGetter)); | ||
| if (tagName != null) { | ||
| // we've got a class node with an `is` property, which tells us that | ||
| // the class we're dealing with is a Stencil component which has | ||
| // already been through the `convertDecoratorsToStatic` transformer. | ||
| return ts.visitEachChild(node, findRenderMethodVisitor, transformCtx); | ||
| } |
There was a problem hiding this comment.
this is the same approach we use in parseStaticComponentMeta to figure out whether a class is a Stencil component or not, see here:
this ensures that we only continue with transforming the syntax tree if we're dealing with a Stencil component, so if you have for instance a class-based React component in your source code it should not be transformed
d6e9eac to
53a92bc
Compare
|
@rwaskiewicz I believe this is ready for another look! I've addressed your comments and added a few more tests to cover some more edge cases, lmk what you think! |
rwaskiewicz
left a comment
There was a problem hiding this comment.
Overall looks good!
I have two asks before we can merge this:
- I had one change I'd like to make to the code in its current state (comment made as a part of this review)
- Did we ever push the PR up containing docs around this functionality?
| expect(rect.getAttribute('transform')).toBe(null); | ||
|
|
||
| root.isOpen = true; | ||
| await waitForChanges(); | ||
| rect = root.querySelector('rect'); | ||
| expect(rect.getAttribute('transform')).toBe('rotate(45 27 27)'); | ||
|
|
||
| root.isOpen = false; | ||
| await waitForChanges(); | ||
| rect = root.querySelector('rect'); |
There was a problem hiding this comment.
I'm not sure I agree with the premise - we may be within our right to claim behavior here, but I don't think the user would necessarily understand the distinction here. They'd just see broken tests that "worked before".
But! That all may be moot - I think in
applying this patch (revert-tests.patch, we can revert these changes safely with the latest round of commits and have the tests still pass 👍
e73e6bf to
7631132
Compare
|
@rwaskiewicz I removed those changes, thanks for checking if they were still necessary! |
|
also I opened a documentation here: stenciljs/site#1330 |
christian-bromann
left a comment
There was a problem hiding this comment.
Struggled a bit to reproduce the problem but with the help of @alicewriteswrongs reproducible example I was able that the issue is resolved with this patch 👍
this commit adds a karma test for #1968, based on the minimal reproduction case that we received for said issue
This adds a new transformer, `performAutomaticKeyInsertion`, which will
add `key` attributes into certain JSX nodes in the `render()` method of
a Stencil component. This will allow the Stencil runtime to make more
accurate decisions about when to create and destroy child nodes during
re-rendering, especially in circumstances where nodes are changing order
and so on.
There are some limitations on when we can safely insert keys without
possibly introducing incorrect behavior. In particular, we don't want to
insert keys in the following situations:
- when a `render` method has more than one return statement
- when a `render` method has a conditional (ternary) expression in it
- when a JSX node is located inside of a JSX expression (i.e. within
curly braces like `<div>{ ... }</div>`)
In each of these cases we don't have the static analysis chops to know
when a given JSX syntax tree node is supposed to correspond to the same
HTML element at runtime, whereas in the 'single return, JSX children
case' like:
```tsx
class Component {
render () {
return <div><img /></div>
}
}
```
We know without a doubt in this case that the `div` and `img` JSX nodes
are always supposed to correspond to the same HTML elements at runtime,
so it's no problem to add keys to them.
The key insertion transformer also does not transform JSX nodes which
are not part of Stencil components, so if a project had, for some
reason, a React component or something in it we will leave it alone.
fixes #1968
fixes #5263
STENCIL-893
1b9e0da to
1df736e
Compare
This functionality was added to Stencil in stenciljs/core#5143
This functionality was added to Stencil in stenciljs/core#5143
This expands the scope of the automatic key insertion feature that was added in #5143. Previously the feature stopped at the border of any `JsxExpression` syntax node (a bit of arbitrary JavaScript wrapped in curly braces and then inserted into JSX) so that if you were doing something like: ```tsx <div>{ someCondition && <span>when condition is true</span>}</div> ``` the compiler would insert a key into the `<div>` syntax tree node but _not_ the node for the `<span>`. We did this to just be a bit cautious with the feature because there are a lot of different things that could be going on within a `JsxExpression` node, and some things which could be written within curly braces which would not be safe to transform, such as the following: ```tsx <div>{ xs.map(x => <span>{ x }</span>) }</div> ``` We wouldn't want to insert a key into the `<span>` syntax tree node because that would result in the dynamically generated `<span>` tags always having the same key attributes. That said, there are some situations where it is safe to insert a key, such as the `condition || <some-tag />` case shown above. This change adds support for inserting keys in those situations. STENCIL-1120
This expands the scope of the automatic key insertion feature that was added in #5143. Previously the feature stopped at the border of any `JsxExpression` syntax node (a bit of arbitrary JavaScript wrapped in curly braces and then inserted into JSX) so that if you were doing something like: ```tsx <div>{ someCondition && <span>when condition is true</span>}</div> ``` the compiler would insert a key into the `<div>` syntax tree node but _not_ the node for the `<span>`. We did this to just be a bit cautious with the feature because there are a lot of different things that could be going on within a `JsxExpression` node, and some things which could be written within curly braces which would not be safe to transform, such as the following: ```tsx <div>{ xs.map(x => <span>{ x }</span>) }</div> ``` We wouldn't want to insert a key into the `<span>` syntax tree node because that would result in the dynamically generated `<span>` tags always having the same key attributes. That said, there are some situations where it is safe to insert a key, such as the `condition || <some-tag />` case shown above. This change adds support for inserting keys in those situations. STENCIL-1120
This expands the scope of the automatic key insertion feature that was added in #5143. Previously the feature stopped at the border of any `JsxExpression` syntax node (a bit of arbitrary JavaScript wrapped in curly braces and then inserted into JSX) so that if you were doing something like: ```tsx <div>{ someCondition && <span>when condition is true</span>}</div> ``` the compiler would insert a key into the `<div>` syntax tree node but _not_ the node for the `<span>`. We did this to just be a bit cautious with the feature because there are a lot of different things that could be going on within a `JsxExpression` node, and some things which could be written within curly braces which would not be safe to transform, such as the following: ```tsx <div>{ xs.map(x => <span>{ x }</span>) }</div> ``` We wouldn't want to insert a key into the `<span>` syntax tree node because that would result in the dynamically generated `<span>` tags always having the same key attributes. That said, there are some situations where it is safe to insert a key, such as the `condition || <some-tag />` case shown above. This change adds support for inserting keys in those situations. STENCIL-1120
This expands the scope of the automatic key insertion feature that was added in #5143. Previously the feature stopped at the border of any `JsxExpression` syntax node (a bit of arbitrary JavaScript wrapped in curly braces and then inserted into JSX) so that if you were doing something like: ```tsx <div>{ someCondition && <span>when condition is true</span>}</div> ``` the compiler would insert a key into the `<div>` syntax tree node but _not_ the node for the `<span>`. We did this to just be a bit cautious with the feature because there are a lot of different things that could be going on within a `JsxExpression` node, and some things which could be written within curly braces which would not be safe to transform, such as the following: ```tsx <div>{ xs.map(x => <span>{ x }</span>) }</div> ``` We wouldn't want to insert a key into the `<span>` syntax tree node because that would result in the dynamically generated `<span>` tags always having the same key attributes. That said, there are some situations where it is safe to insert a key, such as the `condition || <some-tag />` case shown above. This change adds support for inserting keys in those situations. STENCIL-1120
…5594) This expands the scope of the automatic key insertion feature that was added in #5143. Previously the feature stopped at the border of any `JsxExpression` syntax node (a bit of arbitrary JavaScript wrapped in curly braces and then inserted into JSX) so that if you were doing something like: ```tsx <div>{ someCondition && <span>when condition is true</span>}</div> ``` the compiler would insert a key into the `<div>` syntax tree node but _not_ the node for the `<span>`. We did this to just be a bit cautious with the feature because there are a lot of different things that could be going on within a `JsxExpression` node, and some things which could be written within curly braces which would not be safe to transform, such as the following: ```tsx <div>{ xs.map(x => <span>{ x }</span>) }</div> ``` We wouldn't want to insert a key into the `<span>` syntax tree node because that would result in the dynamically generated `<span>` tags always having the same key attributes. That said, there are some situations where it is safe to insert a key, such as the `condition || <some-tag />` case shown above. This change adds support for inserting keys in those situations. STENCIL-1120
Putting up the code for this now so we can start looking at it!
What is the current behavior?
We have a number of issues in the backlog relating to shortcomings in how children are reconciled across virtual DOM re-renders at present. For example, #1968 is caused by an interaction between a failure to correctly recognize the same child across a re-render and another issue with slot content relocation in a
scopedcontext.This implements an approach to addressing a subset of these issues by automatically inserting
keyattributes at compile-time, making it easier at run-time for the virtual DOM to determine when two children of a virtual DOM node should be matched up with each other.What is the new behavior?
Now as part of transpilation Stencil will insert randomly-generated
keyattributes into JSX syntax tree nodes. This allows the more-accurate key-based child reconciliation algorithm to be used almost all the time, fixing some edge cases where previously the Stencil runtime would accidentally misplace a child, failing to, for instance, correctly match up an old and new vdom node when nodes were reordered.Does this introduce a breaking change?
I don't believe it does!
Testing
I have added some tests which exercise the change and also ensured that all of our existing tests are passing without issue.
I also built and ran this in Framework, where all of the unit tests are passing and some smoke testing of components (the datetime in particular) doesn't reveal any potential problems.
But! For testing this I think it would be a good idea to check out the reproduction case from #1968. Then:
@stencil/core@latestnpm run build && npm pack