fix(runtime): patch `removeChild` for `scoped` components by tanner-reits · Pull Request #5148 · stenciljs/core

tanner-reits · 2023-12-08T21:15:31Z

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.

Follow the React framework wrapper guide to get a working React app with Stencil proxied components.
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)
Run the React app and interact with the button to remove nodes. The component will disappear and a console error will be thrown
Install a build of this branch into the Stencil component library.
Enable extras.experimentalSlotFixes in the Stencil config.
Re-build the Stencil library and React component library
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.

This commit adds a patch for an HtmlElement's `removeChild` method Fixes: #3278 #2259 STENCIL-937 Co-authored-by: johnjenkins <johnljenkins@hotmail.com>

github-actions · 2023-12-08T21:17:20Z

`--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

Two small notes

tanner-reits · 2023-12-08T21:16:53Z

src/runtime/dom-extras.ts

  patchSlotInsertAdjacentText(hostElementPrototype);
  patchTextContent(hostElementPrototype, descriptorPrototype);
  patchChildSlotNodes(hostElementPrototype, descriptorPrototype);
+  patchSlotRemoveChild(hostElementPrototype);


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.

tanner-reits · 2023-12-08T21:17:54Z

src/runtime/vdom/vdom-render.ts

 * @param elm the element of interest
 */
-const updateFallbackSlotVisibility = (elm: d.RenderNode) => {
+export const updateFallbackSlotVisibility = (elm: d.RenderNode) => {


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

src/runtime/dom-extras.ts

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

Tanner Reits and others added 2 commits December 8, 2023 14:23

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>

add e2e tests

0aba8c9

tanner-reits commented Dec 8, 2023

View reviewed changes

tanner-reits marked this pull request as ready for review December 8, 2023 21:26

tanner-reits requested a review from a team as a code owner December 8, 2023 21:26

tanner-reits requested review from christian-bromann and rwaskiewicz December 8, 2023 21:26

rwaskiewicz approved these changes Dec 11, 2023

View reviewed changes

src/runtime/dom-extras.ts Outdated Show resolved Hide resolved

christian-bromann approved these changes Dec 11, 2023

View reviewed changes

PR feedback

a356bfe

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>

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 added this pull request to the merge queue Dec 12, 2023

Merged via the queue into main with commit 956c196 Dec 12, 2023

tanner-reits deleted the treits/fix/patch-remove-child branch December 12, 2023 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix(runtime): patch `removeChild` for `scoped` components#5148
tanner-reits merged 3 commits intomainfrom
treits/fix/patch-remove-child

tanner-reits commented Dec 8, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Dec 8, 2023 •

edited

Loading

Uh oh!

tanner-reits left a comment

Uh oh!

tanner-reits Dec 8, 2023

Uh oh!

tanner-reits Dec 8, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tanner-reits commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

Uh oh!

github-actions bot commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

--strictNullChecks error report

reports and statistics

Unused exports report

Uh oh!

tanner-reits left a comment

Choose a reason for hiding this comment

Uh oh!

tanner-reits Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

tanner-reits Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tanner-reits commented Dec 8, 2023 •

edited

Loading

github-actions bot commented Dec 8, 2023 •

edited

Loading

`--strictNullChecks` error report