fix(compiler): consistently generate additional type files #4938
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/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/vdom/vdom-render.ts | 20 |
| src/runtime/client-hydrate.ts | 19 |
| src/screenshot/connector-base.ts | 19 |
| 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 | 424 |
| TS2322 | 398 |
| TS18048 | 310 |
| TS18047 | 100 |
| 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 12 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/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 |
HansClaasen
force-pushed
the
hc/compiler-side-effect
branch
from
15e5233
to
ccd3f6a
Compare
tanner-reits
requested changes
Oct 18, 2023
There was a problem hiding this comment.
@HansClaasen I think we're good to make the change to avoid the early return, but we'll want to keep the result of generateAppTypes around and use it as the return value here for the context where runTsProgram() gets executed
HansClaasen
force-pushed
the
hc/compiler-side-effect
branch
from
ccd3f6a
to
b4fa959
Compare
The if statement doesn't really make sense to me as I would assume you would return early in case no types have changed. However this is causing an interesting side effect where the filesystem in memory determines to delete the directory because the directory and the file in it is not being written. A second execution actually re-creates the file. It it has hard to determine the origin of this change. Likely this wasn't causing any harm until the memory FS logic started to clear out directories that had no files written to them. This has potentially a small performance impact since types are now always generated which they would have been before too except the first time. However if we are not writing the file to memory, the directory will be cleaned up by the memfs system when we commit changes in [`writeBuild`](https://github.com/ionic-team/stencil/blob/b91f94a122dc9add876f0277f99afc92c96e66dd/src/compiler/build/write-build.ts#L24). Fixes ionic-team#4716
HansClaasen
force-pushed
the
hc/compiler-side-effect
branch
from
b4fa959
to
5c2ece6
Compare
|
@tanner-reits thanks for your review. I made necessary changes, please let me know if they align with what you had in mind. Thank you! |
rwaskiewicz
approved these changes
Oct 19, 2023
|
I'm gonna wait for @tanner-reits to approve and have him merge this, but I think we can approve the CI workflow to run and verify everything looks good there as well |
tanner-reits
approved these changes
Oct 19, 2023
2 tasks
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?
The if statement doesn't really make sense to me as I would assume you would return early in case no types have changed. However this is causing an interesting side effect where the filesystem in memory determines to delete the directory because the directory and the file in it is not being written. A second execution actually re-creates the file.
It it has hard to determine the origin of this change. Likely this wasn't causing any harm until the memory FS logic started to clear out directories that had no files written to them.
This has potentially a small performance impact since types are now always generated which they would have been before too except the first time. However if we are not writing the file to memory, the directory will be cleaned up by the memfs system when we commit changes in
writeBuild.Fixes #4716
GitHub Issue Number: #4716
What is the new behavior?
With this patch, we always generate additional type files for the component, including for the first type generation.
Does this introduce a breaking change?
Testing
No tests added to verify this new behavior given there aren't any tests for part of the code base already. Given that we don't really know the expected behavior for a lot of these parts I am not sure about the value of adding unit tests for the current state. Happy to do so if you think it does make sense.
Other information