fix(runtime): throw proper error if component is loaded with invalid runtime #5675
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
| 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/client-hydrate.ts | 20 |
| src/testing/puppeteer/puppeteer-element.ts | 20 |
| src/screenshot/connector-base.ts | 19 |
| src/runtime/vdom/vdom-render.ts | 17 |
| src/dev-server/request-handler.ts | 15 |
| src/compiler/prerender/prerender-optimize.ts | 14 |
| src/compiler/sys/stencil-sys.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 |
| src/compiler/transformers/transform-utils.ts | 12 |
| src/compiler/transpile/transpile-module.ts | 12 |
| src/mock-doc/test/attribute.spec.ts | 12 |
Our most common errors
| Typescript Error Code | Count |
|---|---|
| TS2322 | 361 |
| TS2345 | 343 |
| TS18048 | 204 |
| TS18047 | 82 |
| TS2722 | 37 |
| TS2532 | 24 |
| TS2531 | 21 |
| TS2454 | 14 |
| TS2790 | 11 |
| TS2352 | 9 |
| TS2769 | 8 |
| TS2538 | 8 |
| TS2416 | 7 |
| 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 |
PR built and packed!Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8789074694/artifacts/1436726702 If your browser saves files to |
alicewriteswrongs
suggested changes
Apr 17, 2024
Comment on lines
+22
to
+25
| /** | ||
| * Fetching logs like this only works in Chromium. Once WebdriverIO v9 is released there | ||
| * will be easier primitives to fetch logs in other browsers as well. | ||
| */ |
There was a problem hiding this comment.
we might want to create a JIRA ticket for upgrading to WebdriverIO v9 and then leave a TODO here to remove the browser.isChromium check
There was a problem hiding this comment.
created STENCIL-1291
|
Thanks @alicewriteswrongs for the feedback, applied suggested changes. |
alicewriteswrongs
approved these changes
May 8, 2024
|
sorry for the delay on the review on this one! |
|
@tanner-reits any feedback on this? |
tanner-reits
approved these changes
May 14, 2024
| /** | ||
| * A namespace for custom type definitions used in a portion of the testing suite | ||
| */ | ||
| export declare namespace SomeTypes { | ||
| type Number = number; | ||
| type String = string; | ||
| } | ||
|
|
||
| export async function setupIFrameTest(htmlFile: string): Promise<HTMLElement> { |
There was a problem hiding this comment.
We can probably use this in the invisible-prehydration tests as well
There was a problem hiding this comment.
I will raise a PR with an update once merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the current behavior?
Given someone imports a component compiled with
dist-custom-elementinto a Stencil project that uses thedistoutput target. In the case that the DCE component shares the same runtime as thedistelements we will run into a runtime error.GitHub Issue Number: #5596
What is the new behavior?
This patch proposes to throw a more meaningful error in these situations.
I wonder if there is a better solution where we detect this at compile time. One idea would be to make the DCE component import an object from the runtime library that is only exported if the runtime is dedicated to DCE components.
Documentation
The
externalRuntimeproperty is documented however, we don't particularly mention this use case.Does this introduce a breaking change?
If the instance is
undefinedin this case an error would be thrown anyway incallRenderas we passinstancealong asundefined.Testing
I added an e2e test for this scenario.
Other information