fix(runtime): relocate slot content from non-shadow to shadow components w/ slot name change#4940
Merged
tanner-reits merged 12 commits intomainfrom Oct 23, 2023
Conversation
added 8 commits
October 10, 2023 17:19
Contributor
|
| 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/vdom/vdom-render.ts | 20 |
| src/runtime/client-hydrate.ts | 19 |
| src/screenshot/connector-base.ts | 19 |
| 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 | 398 |
| TS18048 | 310 |
| TS18047 | 100 |
| 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 12 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/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 |
added 2 commits
October 17, 2023 20:14
tanner-reits
commented
Oct 18, 2023
Contributor
Author
tanner-reits
left a comment
There was a problem hiding this comment.
Just noting one thing. Also, the added test was pulled directly from the associated issue
Comment on lines
+1023
to
+1024
| nodeToRelocate.nodeType === NODE_TYPE.ElementNode && | ||
| slotRefNode.parentElement?.shadowRoot != null && |
Contributor
Author
There was a problem hiding this comment.
Couple things here for context:
- I decided to just ignore non-element nodes for now (i.e. comment nodes and text nodes). Per our discussion, the edge-case where these types of nodes is relevant is highly unlikely, so seems okay to hold on that until we have a need to fix it.
- We check the
shadowRoothere because we only need to deal with these slotslot(yes, the double "slot" there is intentional) values if we're moving the content into a shadow component since the native shadow DOM will handle things from there.
alicewriteswrongs
approved these changes
Oct 23, 2023
Base automatically changed from
treits/fix/hidden-slots-after-relocation
to
main
October 23, 2023 20:16
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the current behavior?
It is possible for content to not be relocated correctly when it is relocated from a non-shadow to a shadow component if a slot is slotted into another slot and the names are different. A basic example may look something like:
In the above example, the "secondary slotted content" text won't render at all since we don't correctly remove the
slotattribute for it to render in the default slot in it's final location.Fixes: #4525
What is the new behavior?
When we move the node under the shadow component, we check the element's
slotattribute against the slot ref node to make sure the two slot names match.Does this introduce a breaking change?
Testing
Manual Testing
Install a build of this branch into the reproduction case for the issue (#4525)
Automated Testing
New e2e test added for reproduction case. All other unit & e2e tests continue to pass
Other information
Only available when the
experimentalSlotFixesconfig extras flag is enabled