-
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(compiler): don't allow shadowRoot getter to avoid hydration issues #5912
Conversation
79fd526
to
b34bba0
Compare
|
Path | Error Count |
---|---|
src/dev-server/index.ts | 37 |
src/dev-server/server-process.ts | 32 |
src/compiler/prerender/prerender-main.ts | 22 |
src/runtime/vdom/vdom-render.ts | 22 |
src/runtime/client-hydrate.ts | 20 |
src/runtime/vdom/test/patch.spec.ts | 19 |
src/runtime/vdom/test/util.spec.ts | 19 |
src/screenshot/connector-base.ts | 19 |
src/testing/puppeteer/puppeteer-element.ts | 19 |
src/dev-server/request-handler.ts | 15 |
src/compiler/prerender/prerender-optimize.ts | 14 |
src/compiler/sys/stencil-sys.ts | 14 |
src/runtime/connected-callback.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/set-value.ts | 13 |
src/compiler/output-targets/output-www.ts | 12 |
src/compiler/transformers/test/parse-vdom.spec.ts | 12 |
src/compiler/transformers/transform-utils.ts | 12 |
Our most common errors
Typescript Error Code | Count |
---|---|
TS2322 | 338 |
TS2345 | 326 |
TS18048 | 190 |
TS18047 | 99 |
TS2722 | 27 |
TS2532 | 19 |
TS2531 | 19 |
TS2790 | 11 |
TS2454 | 10 |
TS2352 | 9 |
TS2769 | 8 |
TS2416 | 7 |
TS2538 | 4 |
TS18046 | 2 |
TS2684 | 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 | 245 | NODE_TYPES |
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 | 116 | Env |
src/compiler/app-core/app-data.ts | 118 | NAMESPACE |
src/compiler/fs-watch/fs-watch-rebuild.ts | 123 | updateCacheFromRebuild |
src/compiler/types/validate-primary-package-output-target.ts | 82 | satisfies |
src/compiler/types/validate-primary-package-output-target.ts | 82 | Record |
src/testing/puppeteer/puppeteer-declarations.ts | 496 | WaitForEventOptions |
src/compiler/sys/fetch/write-fetch-success.ts | 7 | writeFetchSuccessSync |
ts.isCallExpression(decorator.expression) && | ||
decorator.expression.arguments.length === 1 && | ||
ts.isObjectLiteralExpression(decorator.expression.arguments[0]) && | ||
decorator.expression.arguments[0].properties.some( | ||
(prop) => ts.isPropertyAssignment(prop) && prop.name.getText() === 'tag', | ||
) |
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.
Is it easier to just check the decorator name?
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.
Yes, but you could potentially import it with any arbitrary name which is why I did this extra step of checking if the decorator has 1 argument which contains a tag
property since this is the same everywhere.
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.
Ah yeah, good point!
What is the current behavior?
We have discovered an issue where a user is running into hydration issues due to the fact that they have defined a component getter like so:
What is the new behavior?
The compiler now detects this class member in a Stencil components and throws a nice error making users aware that this is not allowed.
Documentation
Does this introduce a breaking change?
This can potentially break for people that do this but don't do any client side hydration.
Testing
Added sufficient unit tests.
Other information
n/a