fix(runtime): slot regressions from experimental slot fixes #5221
Conversation
Nests conditional for applying method patches on scoped components within `experimentalSlotFixes` flag check to avoid applying patches if conditional falls through to `else` block
This commit updates the patched `appendChild` method (for scoped components) to force a re-render on execution. This matches the behavior when using the native implementation
|
| 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/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/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 |
Our most common errors
| Typescript Error Code | Count |
|---|---|
| TS2345 | 367 |
| TS2322 | 365 |
| TS18048 | 230 |
| TS18047 | 99 |
| TS2722 | 37 |
| TS2532 | 26 |
| TS2531 | 23 |
| TS2454 | 14 |
| TS2352 | 12 |
| TS2790 | 11 |
| TS2769 | 8 |
| TS2538 | 8 |
| TS2344 | 6 |
| TS2416 | 6 |
| 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 |
| // Check if there is fallback content that should be hidden | ||
| updateFallbackSlotVisibility(this); | ||
| return; | ||
| // Fore a re-render of the host element | ||
| forceUpdate(this); |
There was a problem hiding this comment.
Can you help me understand why we need to do force an update here?
There was a problem hiding this comment.
Note: I want a little additional clarification here, after reading the git commit msg
There was a problem hiding this comment.
I ran into an issue when testing this out with Framework screenshot tests. For some components that had appendChild patched, the component would include the append child node after the call until something triggered a re-render of the component (this was as simple as hovering the component). This made it so the component would immediately re-render to reflect the changes and get the tests to pass
| @@ -2,6 +2,7 @@ import { Component, h } from '@stencil/core'; | |||
|
|
|||
| @Component({ | |||
| tag: 'dom-reattach-clone-deep-slot', | |||
| scoped: true, | |||
There was a problem hiding this comment.
For my own understanding, why do we need to add scoped to the components in this test suite?
There was a problem hiding this comment.
The test uses the cloneNode method on a Stencil component, but with the changes to the bootstrap methods in this PR, we only patch native methods for scoped components when experimentalSlotFixes is enabled.
There was a problem hiding this comment.
Don't anything in addition to what @rwaskiewicz already pointed out 👍
What is the current behavior?
A few changes made behind the
experimentalSlotFixesflag were resulting in screenshot test failures in the Ionic FrameworkGitHub Issue Number: N/A
What is the new behavior?
experimentalSlotFixesis enabled.appendChildpatch to force a re-render of the patched elementSee commit messages for more information
Does this introduce a breaking change?
Testing
Automated tests
Other information