feat(compiler): perform automatic key insertion in more situations#5594
feat(compiler): perform automatic key insertion in more situations#5594alicewriteswrongs merged 1 commit intomainfrom
Conversation
|
| 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 |
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 |
56814bc to
2210b4f
Compare
rwaskiewicz
left a comment
There was a problem hiding this comment.
All in all, looks/works good! Just a couple minor asks
| // 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)) { |
There was a problem hiding this comment.
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?
| // 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 |
There was a problem hiding this comment.
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 😆).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
2210b4f to
405e8a8
Compare
rwaskiewicz
left a comment
There was a problem hiding this comment.
LGTM - one non-blocking nit. Comments make sense 🙂
|
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 |
|
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
78f163f to
f1b3faa
Compare
This expands the scope of the automatic key insertion feature that was added in #5143. Previously the feature stopped at the border of any
JsxExpressionsyntax node (a bit of arbitrary JavaScript wrapped in curly braces and then inserted into JSX) so that if you were doing something like: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 aJsxExpressionnode, and some things which could be written within curly braces which would not be safe to transform, such as the following: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?
Testing
Other information