Skip to content
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

Build: Enable TypeScript skipDefaultLibCheck #63056

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Jul 2, 2024

What?

Enabling this configuration should improve TypeScript build performance with few or no downsides.

Why?

This skips checking types that are bundled with TypeScript. We should
be able to trust these types, so they can be used without checking them.

Many packages use the DOM types and this appeared as a hot spot when
analyzing the TypeScript build.

Adding this configuration should improve TypeScript build performance.

Benchmark

Run ./node_modules/.bin/tsc --build --force, which is a build without caches:

Command Mean [s] Min [s] Max [s] Relative
branch 80.844 ± 2.019 78.665 83.717 1.00
trunk 91.972 ± 1.819 88.670 94.230 1.14 ± 0.04

That's a ~10s build improvement on my machine.

Testing Instructions

CI passes.

This skips _checking_ types that are bundled with TypeScript. We should
be able to trust these types, so they can be used without checking them.

Many packages use the DOM types and this appeared as a hot spot when
analyzing the TypeScript build.

Adding this configuration should improve TypeScript build performance.
@sirreal sirreal added the [Type] Build Tooling Issues or PRs related to build tooling label Jul 2, 2024
@sirreal sirreal marked this pull request as ready for review July 2, 2024 14:42
Copy link

github-actions bot commented Jul 2, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement 👍 Just one little question before we 🚀

@@ -14,6 +14,8 @@
"emitDeclarationOnly": true,
"isolatedModules": true,

"skipDefaultLibCheck": true,
Copy link
Member

@tyxla tyxla Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TS doc recommends that we use skipLibCheck instead. Any good reason to prefer skipDefaultLibCheck?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that, I'm not sure why they recommend that.

skipLibCheck ensures that the libraries we depend on typecheck. I understand that skipDefaultLibCheck only skips checking bundle libs (like DOM, ES2020, etc.). These libs are bundled with TypeScript and they should be the highest quality.

With other libs that come from other node_modules, it seems best to check their types especially because if we don't check these files, the errors can appear in other projects that have skipLibCheck disabled. We've seen that packages become stale or have conflicting types that we need to address, I dealt with a lot of this recently in #61486 and related PRs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. Seems like TS docs need some love!

@sirreal sirreal requested a review from tyxla July 2, 2024 16:29
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing a ~8s and ~10s improvement respectively on 2 runs 👍 Let's go for it 🚀

@sirreal sirreal merged commit 6a5e8df into trunk Jul 2, 2024
76 checks passed
@sirreal sirreal deleted the add/ts-skip-default-lib-check branch July 2, 2024 16:58
@github-actions github-actions bot added this to the Gutenberg 18.8 milestone Jul 2, 2024
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Jul 18, 2024
Skip checking types that are bundled with TypeScript. We should
be able to trust these types, so they can be used without checking.

Many packages use the DOM types and this appeared as a hot spot when
analyzing the TypeScript build.

Adding this configuration should improve TypeScript build performance.

---

Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants