Only add HYDRATED_CSS when cmpTags are set #3771

raldenhoven · 2022-10-26T12:50:19Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Unit tests (npm test) were run locally and passed
E2E Tests (npm run test.karma.prod) were run locally and passed
Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

An invalid piece of CSS is injected when cmpTags are empty. If this happens in JSDOM it will throw an invalid css error.

GitHub Issue Number: N/A

What is the new behavior?

The CSS will only be added to the head when cmpTags are defined (making the css always valid).

Does this introduce a breaking change?

Yes
No

Testing

Manually tested the change in the compiled code, no error where thrown again.

Other information

github-actions · 2022-10-26T12:52:22Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1391 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/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 15 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/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	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

rwaskiewicz · 2022-10-26T13:15:20Z

Hey @raldenhoven 👋

Before we move forward with this PR, we'd like a little more information to better contextualize the need for this change. From the GH PR summary:

An invalid piece of CSS is injected when cmpTags are empty. If this happens in JSDOM it will throw an invalid css error.

Can you provide us examples of invalid CSS that was being produced?
Can you explain your setup using JSDOM? A minimal reproduction case of the issue would go a long way in helping us understand things

Manually tested the change in the compiled code, no error where thrown again.

Can you provide testing steps in the GH Summary? Ideally against that minimal reproduction case? Unfortunately from a reproducability standpoint, it's difficult for the team to verify this works as expected without a step-by-step guide here.

Thanks!

raldenhoven · 2022-10-28T15:31:21Z

Hey @rwaskiewicz I can understand.. I added a test that shows the broken css that is being injected by the bootstrap-lazy method.

The easiest way to reproduce this bug was to call defineCustomElements multiple times on a page, hope this helps!

rwaskiewicz · 2022-10-31T18:07:15Z

Hey @raldenhoven 👋

I have a little more context now, thanks for the test! Can you please create a minimal reproduction case for us to look at? While the test helps, it's important for use to have as much context as possible in a minimal reproduction case that includes a Stencil config, similar usage to the real world problem, etc.; npx init stencil component my-library should be able to get you started with a Stencil starter component library. If you could build off that it'd be a big help!

raldenhoven · 2022-11-02T10:52:53Z

@rwaskiewicz The reproduction case:
https://github.com/raldenhoven/stencil-pr-3771

rwaskiewicz · 2022-11-09T13:54:16Z

Thanks @raldenhoven! I'm going to label this for someone to take a closer look

When cmpTags are empty the added css is invalid.

tanner-reits · 2024-01-17T21:59:29Z

@raldenhoven I know this has been sitting for a while, but we've finally had a chance to look at it!

As you noted, the issue is that defineCustomElements() is being called multiple times. When running the app, it is called in the main.tsx file and the auto-generated file from the reactOutputTarget. For testing, it happens in the auto-generated file and the beforeEach callback in the test suite. I'm not sure if you intentionally set it up this way to showcase the issue, but the duplicate calls can be solved by either 1) removing the manual calls in main.tsx and the beforeEach() callback or 2) setting includeDefineCustomElements: false in the config for the React output target in the Stencil config.

That being said, I do think we could integrate this PR into the codebase as a safety mechanism if developers do end up in this situation. If you want to address the merge conflicts and make sure CI passes, I'd be happy to officially review this and bring it back to the team. Alternatively, I'm happy to finalize this work if you are not willing/able (or if we don't hear back from ya in a while 😉 ).

raldenhoven · 2024-01-19T14:45:35Z

@tanner-reits I'll make some time next week to update this MR! Thanks.

This commit fixes an issue where invalid CSS could be generated and inserted into the DOM if `bootstrapLazy` was called with no component metadata or if no new components are registered in the custom element registry (i.e. it is called multiple times with the same metadata). STENCIL-632 Closes: #3771 Co-authored-by: Robin Aldenhoven <[email protected]>

tanner-reits · 2024-01-29T19:52:14Z

@raldenhoven Just wanted to let ya know I created #5305 to resolve this and added you as a co-author on the main commit! Gonna close this PR out, but we should have the new PR reviewed and merged soon. Hope this helps!

This commit fixes an issue where invalid CSS could be generated and inserted into the DOM if `bootstrapLazy` was called with no component metadata or if no new components are registered in the custom element registry (i.e. it is called multiple times with the same metadata). STENCIL-632 Closes: #3771 Co-authored-by: Robin Aldenhoven <[email protected]>

…ags (#5305) * fix(runtime): only generate lazy build CSS when there are component tags This commit fixes an issue where invalid CSS could be generated and inserted into the DOM if `bootstrapLazy` was called with no component metadata or if no new components are registered in the custom element registry (i.e. it is called multiple times with the same metadata). STENCIL-632 Closes: #3771 Co-authored-by: Robin Aldenhoven <[email protected]> * remove prehydration tests This commit removes some "invisible prehydration" tests that no longer work based on changes in 2559917. They no longer work because `bootstrapLazy` is called with the same compiler metadata so styles are not re-inserted after being manually removed by calls to `tearDownStylesScripts` in the respective test files * chore(karma): remove additional invisilbePrehydration infra * mark todo --------- Co-authored-by: Robin Aldenhoven <[email protected]> Co-authored-by: Ryan Waskiewicz <[email protected]>

alicewriteswrongs · 2024-02-05T16:03:18Z

The fix for this was just released today in Stencil v4.12.1!

raldenhoven requested a review from a team as a code owner October 26, 2022 12:50

rwaskiewicz added the Awaiting Reply This PR or Issue needs a reply from the original reporter. label Oct 26, 2022

ionitron-bot bot removed the Awaiting Reply This PR or Issue needs a reply from the original reporter. label Oct 28, 2022

rwaskiewicz added the triage label Oct 31, 2022

rwaskiewicz added ionitron: needs reproduction This PR or Issue does not have a reproduction case URL and removed triage labels Oct 31, 2022

rwaskiewicz self-assigned this Nov 8, 2022

rwaskiewicz added triage and removed ionitron: needs reproduction This PR or Issue does not have a reproduction case URL labels Nov 8, 2022

rwaskiewicz added Bug: Validated This PR or Issue is verified to be a bug within Stencil and removed triage labels Nov 9, 2022

rwaskiewicz removed their assignment Nov 17, 2022

rwaskiewicz mentioned this pull request May 18, 2023

improvement: validate empty cmpTags. #4391

Closed

rwaskiewicz force-pushed the fix-hydrated-css branch from 95d89b6 to a57b905 Compare July 20, 2023 18:55

raldenhoven and others added 3 commits November 3, 2023 16:24

Only add HYDRATED_CSS when cmpTags are set

f0f4dd3

When cmpTags are empty the added css is invalid.

Added example

b07140c

format/lint test file

df9d35f

rwaskiewicz force-pushed the fix-hydrated-css branch from e26e704 to df9d35f Compare November 3, 2023 20:24

rwaskiewicz mentioned this pull request Nov 10, 2023

fix: prevent broken stylesheet being added to DOM #2821

Closed

tanner-reits mentioned this pull request Jan 29, 2024

fix(runtime): only generate lazy build CSS when there are component tags #5305

Merged

2 tasks

tanner-reits closed this Jan 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only add HYDRATED_CSS when cmpTags are set #3771

Only add HYDRATED_CSS when cmpTags are set #3771

raldenhoven commented Oct 26, 2022

github-actions bot commented Oct 26, 2022 •

edited

Loading

rwaskiewicz commented Oct 26, 2022

raldenhoven commented Oct 28, 2022

rwaskiewicz commented Oct 31, 2022

raldenhoven commented Nov 2, 2022

rwaskiewicz commented Nov 9, 2022

tanner-reits commented Jan 17, 2024

raldenhoven commented Jan 19, 2024

tanner-reits commented Jan 29, 2024

alicewriteswrongs commented Feb 5, 2024

Only add HYDRATED_CSS when cmpTags are set #3771

Only add HYDRATED_CSS when cmpTags are set #3771

Conversation

raldenhoven commented Oct 26, 2022

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

github-actions bot commented Oct 26, 2022 • edited Loading

--strictNullChecks error report

reports and statistics

Unused exports report

rwaskiewicz commented Oct 26, 2022

raldenhoven commented Oct 28, 2022

rwaskiewicz commented Oct 31, 2022

raldenhoven commented Nov 2, 2022

rwaskiewicz commented Nov 9, 2022

tanner-reits commented Jan 17, 2024

raldenhoven commented Jan 19, 2024

tanner-reits commented Jan 29, 2024

alicewriteswrongs commented Feb 5, 2024

github-actions bot commented Oct 26, 2022 •

edited

Loading

`--strictNullChecks` error report