fix(runtime): slotted content order with sibling elements#4994
fix(runtime): slotted content order with sibling elements#4994tanner-reits merged 9 commits intomainfrom
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/build/build-stats.ts | 27 |
| src/compiler/output-targets/dist-lazy/generate-lazy-module.ts | 25 |
| src/compiler/style/test/optimize-css.spec.ts | 23 |
| src/testing/puppeteer/puppeteer-element.ts | 23 |
| src/compiler/prerender/prerender-main.ts | 22 |
| src/runtime/client-hydrate.ts | 19 |
| 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/build/build-finish.ts | 13 |
| src/compiler/prerender/prerender-queue.ts | 13 |
Our most common errors
| Typescript Error Code | Count |
|---|---|
| TS2345 | 418 |
| TS2322 | 395 |
| TS18048 | 310 |
| TS18047 | 101 |
| TS2722 | 38 |
| TS2532 | 34 |
| TS2531 | 23 |
| TS2454 | 14 |
| TS2352 | 13 |
| TS2769 | 10 |
| TS2790 | 10 |
| TS2538 | 8 |
| TS2344 | 5 |
| TS2416 | 4 |
| TS2493 | 3 |
| TS18046 | 2 |
| TS2684 | 1 |
| TS2488 | 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 | 232 | resolve |
| src/utils/index.ts | 246 | normalize |
| 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 | 62 | satisfies |
| src/compiler/types/validate-primary-package-output-target.ts | 62 | 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.
two questions!
rwaskiewicz
left a comment
There was a problem hiding this comment.
one non-blocking question
| // | ||
| // If there is no node immediately following the slot reference node, then we will just | ||
| // end up appending the node as the last child of the parent. | ||
| let insertBeforeNode = slotRefNode.nextSibling as d.RenderNode | null; |
There was a problem hiding this comment.
Do we need this type assertion? I'm not sure what purpose it's serving here ATM
There was a problem hiding this comment.
I saw #4994 (comment) after I approved this, but I'm not sure I understand (but also, my brain is actively turning to goo)
There was a problem hiding this comment.
We need to cast it since we may reassign this value later and the types won't match. nextSibling is of type ChildNode | null | undefined, but in the while loop we reassign a node that could be RenderNode | null | undefined. I tried to update our RenderNode interface to type nextSibling and previousSibling as RenderNode | null, but that broke types in some other places, so didn't wanna fight with that right now.
2acf4f3 to
71d7516
Compare
What is the current behavior?
It is possible for slot content that gets relocated to a nested component to appear in the wrong order with the nested component's other non-slotted elements. This was due to logic we had for keeping relocated content in the correct order within a slot executing when it shouldn't and changing our relocation node.
Fixes: #2997
What is the new behavior?
Does this introduce a breaking change?
Testing
All existing unit and e2e tests continue to pass. Added an e2e test for the reproduction case from the linked issue
Other information
This is only available with the
experimentalSlotFixesextra config flag is active