-
Notifications
You must be signed in to change notification settings - Fork 795
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.
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
fix(runtime): only generate lazy build CSS when there are component tags #5305
Conversation
|
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 | 17 |
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 | 366 |
TS2322 | 362 |
TS18048 | 224 |
TS18047 | 99 |
TS2722 | 37 |
TS2532 | 26 |
TS2531 | 23 |
TS2454 | 14 |
TS2790 | 11 |
TS2352 | 10 |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@tanner-reits It looks like the BS test "invisible-prehydration-default" is consistently failing here. IIRC, that test was also failing in #3771 (logs from 3771). Would you mind taking a look? |
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]>
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
2559917
to
8ee3910
Compare
@rwaskiewicz Removed those tests as discussed |
@tanner-reits I found a few additional files for the invisible prehydration testing that I removed in cdafaa0 - can you & @christian-bromann PTAL? Otherwise, this looks good to me, I had just one non-blocking comment |
if (BUILD.invisiblePrehydration && (BUILD.hydratedClass || BUILD.hydratedAttribute)) { | ||
dataStyles.innerHTML += cmpTags + HYDRATED_CSS; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking - Can we add a TODO to get this part under unit test?
@rwaskiewicz Thanks for that, LGTM. Wasn't sure if we'd wanna bring these tests back if the ability to remove an element from the registry became a thing, but I suppose we can revert all those changes if that time comes. |
What is the current behavior?
If a developer accidentally calls
defineCustomElements()
multiple times with the same component metadata, invalid CSS will be generated and injected into the DOM. This happens because we only create acmpTags
array based on new component tags added to the Custom Element registry, but we do not check if the array has a length before appending additional CSS rules to the tag selectors.Closes: #3771
What is the new behavior?
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).Documentation
N/A
Does this introduce a breaking change?
Testing
Automated testing
Added some new unit tests to
bootstrap-lazy.spec.tsx
Manual testing
This can be tested using the reproduction in this comment from #3771 and a build of this branch.
Note: When testing I was experiencing some caching issues with the wrong version of the runtime being used so you may need to nuke
node_modules
and lock files when installing this build into the projectOther information