Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(runtime): patch removeChild for scoped components #5148

Merged
merged 3 commits into from
Dec 12, 2023
Merged

fix(runtime): patch removeChild for scoped components #5148

merged 3 commits into from
Dec 12, 2023

Conversation

tanner-reits
Copy link
Member

@tanner-reits tanner-reits commented Dec 8, 2023 •
edited
Loading

What is the current behavior?

Stencil does not patch the HtmlElement removeChild() method. This is causing issues in some of our Framework wrappers as frameworks are using these methods during rendering dynamic content (like looping through arrays and generating/removing elements).

Fixes: #3278, #2259

What is the new behavior?

We provide a patch for removeChild() that works with our pseudo-slot behavior for scoped components.

Does this introduce a breaking change?

  • Yes
  • No

Testing

Manual testing
Gonna give abbreviated testing steps since most of it can be achieved following the React framework wrapper guide and the associated issues' repro cases.

  1. Follow the React framework wrapper guide to get a working React app with Stencil proxied components.
  2. Create the Stencil components and React app from this repro from bug: Failed to execute 'removeChild' on 'Node' #3278 or the PR summary of React fails to manage children in Stencil <slot /> #2259 (Note: make all components scoped)
  3. Run the React app and interact with the button to remove nodes. The component will disappear and a console error will be thrown
  4. Install a build of this branch into the Stencil component library.
  5. Enable extras.experimentalSlotFixes in the Stencil config.
  6. Re-build the Stencil library and React component library
  7. Repeat step 3 and notice that no console errors are thrown and the nodes are removed correctly 🎉

Automated testing
Existing unit & e2e tests continue to pass
Added e2e tests for the patched method.

Other information

Only available with the extras.experimentalSlotFixes config flag enabled.

Only available for Stencil component with scoped: true encapsulation.

tanner-reits and others added 2 commits December 8, 2023 14:23
@tanner-reits @johnjenkins
fix(runtime): add patch for removeChild on scoped components
34bca0f 
This commit adds a patch for an HtmlElement's `removeChild` method

Fixes: #3278 #2259

STENCIL-937

Co-authored-by: johnjenkins <johnljenkins@hotmail.com>
@tanner-reits
add e2e tests
0aba8c9
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 8, 2023 •
edited
Loading

--strictNullChecks error report

Typechecking with --strictNullChecks resulted in 1324 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files
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/style/test/optimize-css.spec.ts 23
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
Our most common errors
Typescript Error Code Count
TS2345 399
TS2322 374
TS18048 286
TS18047 102
TS2722 37
TS2532 30
TS2531 23
TS2454 14
TS2352 13
TS2790 10
TS2769 8
TS2538 8
TS2416 6
TS2344 5
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

tanner-reits
tanner-reits commented Dec 8, 2023
View reviewed changes
Copy link
Member Author

@tanner-reits tanner-reits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small notes

src/runtime/dom-extras.ts
@@ -19,6 +20,7 @@ export const patchPseudoShadowDom = (
patchSlotInsertAdjacentText(hostElementPrototype);
patchTextContent(hostElementPrototype, descriptorPrototype);
patchChildSlotNodes(hostElementPrototype, descriptorPrototype);
patchSlotRemoveChild(hostElementPrototype);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanna call out that this will only be patched for scoped components. The associated issues had some examples using no encapsulation, but we're moving away from patching this behavior for that case.

src/runtime/vdom/vdom-render.ts
@@ -695,7 +695,7 @@ export const patch = (oldVNode: d.VNode, newVNode: d.VNode) => {
*
* @param elm the element of interest
*/
const updateFallbackSlotVisibility = (elm: d.RenderNode) => {
export const updateFallbackSlotVisibility = (elm: d.RenderNode) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to export this to use in the patches in dom-extras. Can move this somewhere else if we want

@tanner-reits tanner-reits marked this pull request as ready for review December 8, 2023 21:26
@tanner-reits tanner-reits requested a review from a team as a code owner December 8, 2023 21:26
rwaskiewicz
rwaskiewicz approved these changes Dec 11, 2023
View reviewed changes
src/runtime/dom-extras.ts Outdated Show resolved Hide resolved
@tanner-reits @rwaskiewicz
PR feedback
a356bfe 
Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>
@tanner-reits tanner-reits linked an issue Dec 12, 2023 that may be closed by this pull request
React fails to manage children in Stencil <slot /> #2259
Closed
@tanner-reits tanner-reits added this pull request to the merge queue Dec 12, 2023
Merged via the queue into main with commit 956c196 Dec 12, 2023
120 checks passed
@tanner-reits tanner-reits deleted the treits/fix/patch-remove-child branch December 12, 2023 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christian-bromann christian-bromann christian-bromann approved these changes

@rwaskiewicz rwaskiewicz rwaskiewicz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: Failed to execute 'removeChild' on 'Node' React fails to manage children in Stencil <slot />
3 participants
@tanner-reits @christian-bromann @rwaskiewicz