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
chore: refactor tsconfigs to use project references #9038
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR, @JamesHenry! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9038 +/- ##
==========================================
+ Coverage 88.04% 88.09% +0.04%
==========================================
Files 410 409 -1
Lines 14186 14175 -11
Branches 4135 4135
==========================================
- Hits 12490 12487 -3
+ Misses 1391 1383 -8
Partials 305 305
Flags with carried forward coverage won't be shown. Click here to find out more.
|
tsconfig.base.json
Outdated
"noImplicitReturns": true, | ||
"paths": {}, | ||
"noUnusedLocals": true, |
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.
can we turn this off?
no-unused-vars
does this without causing TS errors (so we can remove the @ts-expect-error
s)
it also means the build won't error out due to a measly unused variable.
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.
Cool yeah I just took a relatively strong starting point inspired by other projects, we can change any of it to whatever's best. It isn't something that is generated or controlled by Nx in any way, it's completely up to us
export { visitorKeys } from './visitor-keys'; | ||
export type { VisitorKeys } from './visitor-keys'; |
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.
we should probably turn on our consistent-type-exports
rule
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.
Yeah all of these type
changes allowed compilation isolatedModules
to work
tsconfig.base.json
Outdated
"lib": ["es2022"], | ||
"module": "NodeNext", | ||
"noEmitOnError": true, | ||
"noFallthroughCasesInSwitch": true, |
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.
ditto here - there's an eslint rule for this
i'd rather not use TS for linting if we can avoid it
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.
Yeah same comment
Tested this out locally and it does seem to work; |
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.
Re-looking at the config layout, I do think you can get away with fewer configs, actually, via some restructuring. But it technically already functions now, of course.
"files": [], | ||
"references": [ | ||
{ | ||
"path": "./tsconfig.build.json" | ||
}, | ||
{ | ||
"path": "./tsconfig.spec.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.
This is interesting; I assume these are the main code and the tests, in which case I would have actually expected you to have tsconfig.json
here be the main code, then have a tsconfig.test.json
(or spec
, that's bikeshedding) separate and then referenced from the repo root
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 idea behind this approach/config structure was as follows:
- A
tsconfig.json
for a project will power its type-checking. This is is not expected to produce any .js etc artifacts. It is logically expected to type-check all files, both production and test files. - An optional
tsconfig.lib.json
(customizable name, in this workspace the convention istsconfig.build.json
) would be used to control atsc
powered build, in which .js etc artifacts are produced. It should naturally not include any test files as part of its build process.- The presence of this file is also how Nx, via the new plugin, can infer that a build "target" should exist for this project (meaning you can run
nx build ast-spec
and it just works, including distributable caching and other core Nx features, without any specific Nx config at the project level.
- The presence of this file is also how Nx, via the new plugin, can infer that a build "target" should exist for this project (meaning you can run
- An optional
tsconfig.spec.json
seemed to make sense as a corollary of thetsconfig.lib.json
/tsconfig.build.json
, but note that its presence is not used to infer any targets. It is only there to pick up all the files that the build config would exclude and possible be passed to a test runner like jest with ts-jest as config.
Does that all make sense? Is there a different/more optimal way to achieve the above objectives around having typecheck and build be separate actions/targets?
"extends": "./tsconfig.build.json", | ||
"extends": "../../tsconfig.base.json", | ||
"compilerOptions": { | ||
"composite": false, | ||
"rootDir": "." | ||
"outDir": "../../dist/out-tsc/ast-spec" |
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 this is a "solution" file, there's actually no reason to extend anything here; these options are all noops.
But, I'm not sure that this layout is quite right, depending on other tooling which may assume that the tsconfig contains the options for the code. Maybe this extension is why you dont' have problems?
"rootDir": "src", | ||
"outDir": "dist", | ||
"emitDeclarationOnly": false, |
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, hm, you're using emitDeclarationOnly
, but only sometimes. It seems simpler to only set noEmit for test projects, then leave all emit on in the base config, then only have tsconfig.json
and tsconfig.spec.json
per-package if you want to split apart tests.
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.
Let me know if my comment here #9038 (comment) alters your view on this please. Having type-checking not produce .js etc artifacts would seem to be desirable, and therefore we would need a way to distinguish between type-checking vs building
"references": [ | ||
{ | ||
"path": "./packages/ast-spec" | ||
}, | ||
{ | ||
"path": "./packages/eslint-plugin" | ||
}, |
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.
One gotcha here may be that TS won't do rebuilds in some situations if all projects are not listed here. Right now, you're using this top-level "solution" that points to other "solutions". Related is: microsoft/TypeScript#30608 / microsoft/TypeScript#55339
"experimentalDecorators": false, | ||
"forceConsistentCasingInFileNames": true, |
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.
A lot of these settings are already their defaults and could be removed for clarity, e.g. allowJs, decorators, pretty, and so on.
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.
Yeah I think that's probably fair... I was shooting for maximum clarity, but probably the fewer things for the average user to think about the better. Y'all are striving for the most logical/useful default values and on the whole users don't need to care about many of the possible options
No description provided.