-
Notifications
You must be signed in to change notification settings - Fork 784
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
feat(compiler): perform automatic key insertion in more situations #5594
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
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
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
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: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 aJsxExpression
node, 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