-
Notifications
You must be signed in to change notification settings - Fork 28
[code-infra] Restructured tsconfig files #666
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
base: master
Are you sure you want to change the base?
Conversation
Bundle size report
Check out the code infra dashboard for more information about this PR. |
f475fa8 to
2a3e603
Compare
2a3e603 to
535df9c
Compare
535df9c to
0787bb8
Compare
0787bb8 to
8040b59
Compare
8040b59 to
eda5add
Compare
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.
Pull Request Overview
This PR restructures the TypeScript configuration files to follow a hierarchical approach. A new base configuration file tsconfig.options.json is introduced that contains common TypeScript compiler options, which is then extended by other configuration files to avoid duplication.
- Created a new base configuration file (
tsconfig.options.json) with common TypeScript compiler options - Replaced duplicated configurations across packages and apps with extends patterns
- Removed the redundant
tsconfig.build.jsonfile from the code-infra package
Reviewed Changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.options.json | New base configuration file with common TypeScript compiler options |
| tsconfig.js.json | New configuration extending base options for JavaScript-enabled projects |
| tsconfig.node.json | Simplified to extend tsconfig.js.json and added babel.config.* to includes |
| packages/docs-infra/tsconfig.json | Updated to extend tsconfig.options.json and removed redundant options |
| packages/code-infra/tsconfig.json | Simplified to extend tsconfig.js.json and updated includes |
| packages/code-infra/tsconfig.build.json | Removed as mentioned in PR description |
| packages/code-infra/src/cli/typescript.mjs | Added --declarationMap false flag to TypeScript command |
| packages/bundle-size-checker/tsconfig.json | Simplified to extend tsconfig.js.json |
| packages/babel-plugin-resolve-imports/tsconfig.json | Simplified to extend tsconfig.js.json |
| packages/babel-plugin-minify-errors/tsconfig.json | Simplified to extend tsconfig.js.json with updated includes |
| package.json | Added @types/babel__core dependency |
| apps/code-infra-dashboard/tsconfig.json | Simplified to extend tsconfig.options.json |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
eda5add to
8c940ce
Compare
|
@Janpot Can I get a review on this ? |
|
When it comes to modularizing tsconfig, I'm not 100% convinced a deep hierarchy is the best solution. See this comment as a reason for allowing arrays in extends. The idea being that you create multiple base configs, each describing an aspect of the environment which you then can freely compose in your app/package configurations. I don't know how that should look exactly, but I'm thinking like e.g. our build tool can export its tsconfig that sets module resolution rules. Maybe we can make a config for when the target is node, and one for when it is browsers. Such that e.g. our node tools extend from |
8c940ce to
a783068
Compare
253d6c2 to
d0ec110
Compare
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.
Pull Request Overview
Copilot reviewed 31 out of 34 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)
packages/code-infra/package.json:55
- The test:copy script references
bin/code-infra.mjswhich no longer exists after moving the entry point tosrc/code-infra.mjs. This should be updated tonode src/code-infra.mjsto match the new bin path.
"test:copy": "rm -rf build && node bin/code-infra.mjs copy-files --glob \"src/cli/*.mjs\" --glob \"src/eslint/**/*.mjs:esm\""
packages/code-infra/package.json:140
- The files array includes "bin" directory which no longer exists after moving the CLI entry point to src/code-infra.mjs. This entry should be removed since the bin file is now in the src directory which is already included in the files array.
"bin",
| "include": ["src/**/*"], | ||
| // removing "cli" since its going to run directly and not really imported anywhere | ||
| "exclude": ["**/*.spec.*", "**/*.test.*", "**/setupVitest.ts"] | ||
| "exclude": ["**/*.spec.*", "**/*.test.*"] |
Copilot
AI
Oct 30, 2025
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.
The comment explaining that CLI files are excluded because they run directly was removed, but the actual exclusion logic changed. The old config excluded setupVitest.ts explicitly; the new one only excludes test files. This could lead to setupVitest.ts being included in the build output unintentionally.
| "exclude": ["**/*.spec.*", "**/*.test.*"] | |
| "exclude": ["**/*.spec.*", "**/*.test.*", "setupVitest.ts"] |
d0ec110 to
ecf2904
Compare
ecf2904 to
6872542
Compare
6872542 to
3f84c2f
Compare
| "build", | ||
| "src", | ||
| "README.md", | ||
| "!**/*.tsbuildinfo" |
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.
Oh wow, didn't know this supported negative patterns
| @@ -1,11 +1,6 @@ | |||
| { | |||
| "extends": "../../tsconfig.json", | |||
| "extends": ["../../tsconfig.base.json", "../../tsconfig.node.json"], | |||
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.
🤔 Since we're bundling the code in docs-infra I'd expected it to have a different moduleResiolution (bundler) than what we'd use in untranspiled code for node (node16)
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.
The only difference between bundler and node16 is that bundler doesn't require extensions for relative imports.
I've updated tsconfig.node.json to have moduleResolution of bundler itself.
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.
The only difference between bundler and node16 is that bundler doesn't require extensions for relative imports.
It also resolves index.js files and probably a few other intricacies. There may also be other options that have to differ between transpiled and untranspiled code. e.g. lib, or target
I've updated tsconfig.node.json to have moduleResolution of bundler itself.
Bundler doesn't correctly reflect untranspiled code. If code is meant to run in node.js untranspiled than it needs to have the node16 moduleResolution flag set for it. This prevents us from writing code that wouldn't run. The Bundler option allows us to be ore lenient since the bundler fixes up the resolution for us.
3f84c2f to
fec204a
Compare
fec204a to
3db5eed
Compare
All apps/packages now extend a common tsconfig.options.json and only add project specific changes in their own configs. Removed tsconfig.build.json in code-infra since there is no build step tehre.
and related fixes
fbaae35 to
6e886c1
Compare
All apps/packages now extend a common
tsconfig.base.jsonand only add project specific changes in their own configs.