feat(compiler): perform automatic key insertion in more situations #5594

alicewriteswrongs · 2024-03-26T18:56:21Z

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:

<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:

<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

What is the current behavior?

GitHub Issue Number: N/A

What is the new behavior?

Documentation

Does this introduce a breaking change?

Yes
No

Testing

Other information

github-actions · 2024-03-26T18:57:50Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1138 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files

Path	Error Count
src/dev-server/index.ts	37
src/dev-server/server-process.ts	32
src/compiler/prerender/prerender-main.ts	22
src/testing/puppeteer/puppeteer-element.ts	21
src/runtime/client-hydrate.ts	20
src/screenshot/connector-base.ts	19
src/runtime/vdom/vdom-render.ts	17
src/dev-server/request-handler.ts	15
src/compiler/prerender/prerender-optimize.ts	14
src/compiler/sys/stencil-sys.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
src/compiler/transformers/transform-utils.ts	12
src/compiler/transpile/transpile-module.ts	12
src/mock-doc/test/attribute.spec.ts	12

Our most common errors

Typescript Error Code	Count
TS2322	361
TS2345	345
TS18048	204
TS18047	82
TS2722	37
TS2532	24
TS2531	21
TS2454	14
TS2790	11
TS2352	9
TS2769	8
TS2538	8
TS2416	7
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

github-actions · 2024-03-26T18:58:09Z

PR built and packed!

Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8571121509/artifacts/1388578614

If your browser saves files to ~/Downloads you can install it like so:

unzip -d ~/Downloads ~/Downloads/stencil-core-4.14.1-dev.1712326253.ce157a6.tgz.zip && npm install ~/Downloads/stencil-core-4.14.1-dev.1712326253.ce157a6.tgz

rwaskiewicz

All in all, looks/works good! Just a couple minor asks

rwaskiewicz · 2024-04-02T12:54:29Z

src/compiler/transformers/automatic-key-insertion/index.ts

-      // we're going to encounter the same problem here that we encounter with
-      // multiple return statements, so we don't try to transform the arms of
-      // the conditional.
+    if (ts.isCallExpression(node) || ts.isPropertyAccessExpression(node) || ts.isConditionalExpression(node)) {


For isPropertyAccessExpression, our tests here still all pass if I remove that part of the conditional - can we add a test for cases where isPropertyAccessExpression is truthy?

rwaskiewicz · 2024-04-02T13:06:01Z

src/compiler/transformers/automatic-key-insertion/index.ts

-      // multiple return statements, so we don't try to transform the arms of
-      // the conditional.
+    if (ts.isCallExpression(node) || ts.isPropertyAccessExpression(node) || ts.isConditionalExpression(node)) {
+      // we're going to encounter the same problems here that we encounter with


Since we've change this conditional to remove isJsxExpression and add isCallExpression+isPropertyAccessExpression, I think it might make sense to update this comment. I remember you doing this original work, but I think some of the context of this comment is a little lost on me now (TBH, I don't remember what the multiple return statement problem is 😆).

yeah, I think that actually the isPropertyAccessExpression check isn't necessary overall, I was thinking of situations like

<div>{ foo.bar(<div />) }</div>

where we might not want to transform the inner <div /> because we don't know what will happen to it (.bar could do anything!) but actually in this case the whole expression foo.bar(<div />) is a CallExpression where the callee is a PropertyAccessExpression. So just checking for isPropertyAccessExpression would only guard against situations like

<div>{ foo.bar }</div>

where the property access expression can't have any children that are JSX nodes anyway...

so anyway yeah I think I will remove it and then fix up the comments

I updated the code here and also fleshed out a comment in the findRenderMethodVisitor helper function to explain a bit more the 'multiple return' problem, lmk what you think!

rwaskiewicz

LGTM - one non-blocking nit. Comments make sense 🙂

src/compiler/transformers/automatic-key-insertion/index.ts

rwaskiewicz · 2024-04-03T20:26:58Z

Actually, @alicewriteswrongs can we hold off on merging this for today? I wanna see if I can get a patch release out to unblock the Lisbon team

rwaskiewicz · 2024-04-04T13:18:13Z

v4.14.1 went out this morning, once this has another ✔️ it should be good to merge. Thanks for holding off on the merge!

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

alicewriteswrongs requested a review from a team as a code owner March 26, 2024 18:56

alicewriteswrongs requested review from christian-bromann and rwaskiewicz March 26, 2024 18:56

alicewriteswrongs marked this pull request as draft March 26, 2024 18:56

alicewriteswrongs marked this pull request as ready for review March 26, 2024 20:00

alicewriteswrongs force-pushed the ap/key-insertion-refinement branch from 56814bc to 2210b4f Compare March 26, 2024 21:23

christian-bromann approved these changes Mar 26, 2024

View reviewed changes

rwaskiewicz requested changes Apr 2, 2024

View reviewed changes

alicewriteswrongs force-pushed the ap/key-insertion-refinement branch from 2210b4f to 405e8a8 Compare April 3, 2024 18:23

alicewriteswrongs requested review from rwaskiewicz and christian-bromann April 3, 2024 18:23

rwaskiewicz approved these changes Apr 3, 2024

View reviewed changes

src/compiler/transformers/automatic-key-insertion/index.ts Outdated Show resolved Hide resolved

alicewriteswrongs force-pushed the ap/key-insertion-refinement branch from 78f163f to f1b3faa Compare April 5, 2024 14:10

alicewriteswrongs enabled auto-merge April 5, 2024 14:10

alicewriteswrongs added this pull request to the merge queue Apr 5, 2024

Merged via the queue into main with commit 8ee071b Apr 5, 2024
121 checks passed

alicewriteswrongs deleted the ap/key-insertion-refinement branch April 5, 2024 14:27

christian-bromann mentioned this pull request May 15, 2024

bug: #5736

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(compiler): perform automatic key insertion in more situations #5594

feat(compiler): perform automatic key insertion in more situations #5594

alicewriteswrongs commented Mar 26, 2024 •

edited

github-actions bot commented Mar 26, 2024 •

edited

github-actions bot commented Mar 26, 2024 •

edited

rwaskiewicz left a comment

rwaskiewicz Apr 2, 2024

rwaskiewicz Apr 2, 2024

alicewriteswrongs Apr 3, 2024 •

edited

alicewriteswrongs Apr 3, 2024

rwaskiewicz left a comment

rwaskiewicz commented Apr 3, 2024

rwaskiewicz commented Apr 4, 2024

feat(compiler): perform automatic key insertion in more situations #5594

feat(compiler): perform automatic key insertion in more situations #5594

Conversation

alicewriteswrongs commented Mar 26, 2024 • edited

What is the current behavior?

What is the new behavior?

Documentation

Does this introduce a breaking change?

Testing

Other information

github-actions bot commented Mar 26, 2024 • edited

--strictNullChecks error report

reports and statistics

Unused exports report

github-actions bot commented Mar 26, 2024 • edited

PR built and packed!

rwaskiewicz left a comment

Choose a reason for hiding this comment

rwaskiewicz Apr 2, 2024

Choose a reason for hiding this comment

rwaskiewicz Apr 2, 2024

Choose a reason for hiding this comment

alicewriteswrongs Apr 3, 2024 • edited

Choose a reason for hiding this comment

alicewriteswrongs Apr 3, 2024

Choose a reason for hiding this comment

rwaskiewicz left a comment

Choose a reason for hiding this comment

rwaskiewicz commented Apr 3, 2024

rwaskiewicz commented Apr 4, 2024

alicewriteswrongs commented Mar 26, 2024 •

edited

github-actions bot commented Mar 26, 2024 •

edited

`--strictNullChecks` error report

github-actions bot commented Mar 26, 2024 •

edited

alicewriteswrongs Apr 3, 2024 •

edited