-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
build: use isolated declarations #20928
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: main
Are you sure you want to change the base?
Conversation
export const buildEnvironmentOptionsDefaults: Readonly< | ||
Partial<BuildEnvironmentOptions> | ||
> = _buildEnvironmentOptionsDefaults |
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 and other similar setups are kinda funky to handle with isolated declarations, and they all relate to configDefaults
, since our source code depends on satisifies
and the inferred type. But it is an issue if we one day export configDefaults
and its type, so the hoops here are as if we're tackling that now as isolated declarations don't know if the exported type is public
eslint.config.js
Outdated
'n/prefer-node-protocol': 'error', | ||
|
||
'@typescript-eslint/ban-ts-comment': 'error', | ||
'@typescript-eslint/consistent-generic-constructors': 'off', |
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.
How about this setting?
'@typescript-eslint/consistent-generic-constructors': [
'error',
'type-annotation',
],
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.
Setting that has 101 errors. I think in some cases type-annotation
makes sense for exported types with isolated declaration, but nested code that doesn't need it writes better if the type is in the constructor
instead. Probably typescript-eslint
needs a new option (or tweak constructor
) that balances for isolated declarations.
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.
Makes sense. It seems typescript-eslint
has languageOptions.parserOptions.isolatedDeclarations: true
for that purpose.
https://github.com/typescript-eslint/typescript-eslint/pull/11351/files#diff-7ca46a3da27e8ad890168dc27f592c83d7f9e12334428396e11b8ca2a6a850adR52-R56
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.
Nice find! That option actually seems to work and we don't have to disable this rule
Description
Commits:
isolatedDeclarations
, and tweak the files/scripts to properly segment which files should enforceisolatedDeclarations
. We don't want it for test files for example.https://github.com/microsoft/ts-fix
. Ugly command but works: