-
Notifications
You must be signed in to change notification settings - Fork 795
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
fix(runtime): allow setting key
attr on nested Stencil components
#5164
Conversation
|
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/output-targets/dist-lazy/generate-lazy-module.ts | 22 |
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 | 18 |
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/runtime/vdom/vdom-annotations.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 |
Our most common errors
Typescript Error Code | Count |
---|---|
TS2345 | 376 |
TS2322 | 373 |
TS18048 | 286 |
TS18047 | 102 |
TS2722 | 37 |
TS2532 | 30 |
TS2531 | 23 |
TS2454 | 14 |
TS2352 | 12 |
TS2790 | 10 |
TS2769 | 8 |
TS2538 | 8 |
TS2344 | 6 |
TS2416 | 6 |
TS2493 | 3 |
TS2488 | 2 |
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 |
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.
one note
@@ -166,6 +166,69 @@ describe('hydrate no encapsulation', () => { | |||
`); | |||
}); | |||
|
|||
it('nested text slot with key', async () => { |
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.
this test is a reproduction of the issue I was seeing, if you wanted to see what happens you could check out this branch locally and either run the test without rebuilding or
git checkout origin/main -- src/runtime/vdom/vdom-render.ts
npm run build
npx jest --clearCache
npx --node-options=--experimental-vm-modules jest --coverage=false -- hydrate-no-encapsulation
which will run this test without the fix so you can see what happens
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 ran this locally, adding what I found for posterity:
● hydrate no encapsulation › nested text slot with key
expect(received).toBe(expected) // Object.is equality
- Expected - 3
+ Received + 1
<cmp-a class="hydrated">
<!--r.1-->
<cmp-b class="hydrated">
- <!--r.2-->
- <!---->
- <!--s.2.0.0.0.-->
+ <!---->
light-dom
<footer></footer>
</cmp-b>
</cmp-a>
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.
Yup that's what I expect to see 👍
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 - added a few non-blocking ocmments
); | ||
} | ||
} | ||
// @ts-ignore |
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 my own knowledge, why do we need this @ts-ignore
? If we need it, can we add a justification/reason why we need it in the comment for future us?
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.
We don't! I copy-pasted an existing test which has this (every test in this file does) and just assumed it was necessary. However, it's not! That's for two reasons:
- we don't type-check the
spec.tsx
files anyway (they're not included intsconfig.include
) so removing these doesn't cause any new errors - it appears that additionally whatever errors these were added to suppress aren't present, since if I add
src/**/*.tsx
toinclude
intsconfig.json
and then start removing these@ts-ignore
comments I don't see any new errors
So I'm going to go ahead and just remove them all from the file
</cmp-a> | ||
`); | ||
|
||
// @ts-ignore |
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 my own knowledge, why do we need this @ts-ignore
? If we need it, can we add a justification/reason why we need it in the comment for future us?
@@ -166,6 +166,69 @@ describe('hydrate no encapsulation', () => { | |||
`); | |||
}); | |||
|
|||
it('nested text slot with key', async () => { |
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 ran this locally, adding what I found for posterity:
● hydrate no encapsulation › nested text slot with key
expect(received).toBe(expected) // Object.is equality
- Expected - 3
+ Received + 1
<cmp-a class="hydrated">
<!--r.1-->
<cmp-b class="hydrated">
- <!--r.2-->
- <!---->
- <!--s.2.0.0.0.-->
+ <!---->
light-dom
<footer></footer>
</cmp-b>
</cmp-a>
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.
Aside from what @rwaskiewicz already pointed out, LGTM 👍
e614583
to
414be47
Compare
414be47
to
6cd5893
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.
6cd5893
to
a92ffe2
Compare
This makes it possible to set a
key
attribute on a nested Stencil component without messing up how hydration works. I noticed that this was not working correctly when working on #5143.What is the current behavior?
There's a regression test which will reproduce the issue if run in the context of
main
. In short, if you have two components like so:You'll get rendering issues where comments that need to be present for slot relocation and whatnot to work aren't appearing. Not good!
What is the new behavior?
This ensures that we only start to check whether
vnode.$key$
matches between vnodes after the first render. This fixes a situation where on the first render the.$key$
property of the old root vnode would always benull
, leading the comparison to result in always re-rendering the component.Does this introduce a breaking change?
Testing
I've run this in Framework (the unit tests) and haven't seen any issues, and I've also checked in out in an example hydrate app I created the other day.
Other information