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: Use typescript-eslint@v6
with reworked configs
#15716
chore: Use typescript-eslint@v6
with reworked configs
#15716
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/54855/ |
I think the end-to-end test failures are not related to these changes? |
@JoshuaKGoldberg Yes, the CI error is probably unrelated. The main branch is also failing, I will take a look. |
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.
It seems that most of the rules we have to explicitly disable are from the stylistic-type-checked
preset. Does it contain more rules that we have to disable, or that we keep enabled? How would our config look if we didn't enable that preset?
It's actually a pretty good mix across the configs. You can see the list of rules enabled in the
|
.eslintrc.cjs
Outdated
@@ -64,12 +68,51 @@ module.exports = { | |||
allowNamedExports: true, | |||
}, | |||
], | |||
"@typescript-eslint/no-unnecessary-type-assertion": "error", |
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.
Why was this rule turned 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.
This line would be redundant because it's now enabled in the recommended-type-checked
config 😄 https://v6--typescript-eslint.netlify.app/rules/no-unnecessary-type-assertion
.eslintrc.cjs
Outdated
"@typescript-eslint/no-namespace": "off", | ||
"@typescript-eslint/no-redundant-type-constituents": "off", | ||
"@typescript-eslint/no-this-alias": "off", | ||
"@typescript-eslint/no-unnecessary-type-assertion": "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.
"@typescript-eslint/no-unnecessary-type-assertion": "off", |
Glad that it is now enabled by default. Well, in that case...
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.
Yay!
I tried this out a bit locally but ran into the following conundrum:
- Without
strictNullChecks: true
intsconfig.base.json
, fixing forno-unnecessary-type-assertion
can cause issues.- Example: The
null as null
cast inpackages/babel-preset-env/src/shipped-proposals.ts
avoid aObject literal's property '"transform-unicode-property-regex"' implicitly has an 'any' type.
complaint
- Example: The
- With
strictNullChecks: true
intsconfig.base.json
, there are quite a few complaints.- Example:
parentPath
inpackages/babel-traverse/src/path/modification.ts
>insertBefore
is potentiallynull
and that causes type errors
- Example:
Can I propose filing a followup to enable strictNullChecks: true
internally, and considering it a blocker for enabling some of these rules?
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.
Perhaps we can temporarily use eslint-disable-next-line
with strictNullChecks: false
?
I remember @JLHwung has a strictNullChecks: true
branch, but it was blocked by ts bug at the time, and it may be outdated now.(I'm willing to give this a try too, if that branch is outdated)
By the way, can null as null
be a special case of no-unnecessary-type-assertion
?
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.
temporarily use
eslint-disable-next-line
withstrictNullChecks: false
That could work. In my experience that can bring positives (more lint rules, sooner) and also some negatives (folks getting confused when new edge case situations like this one pop up). What do you think?
can
null as null
be a special case ofno-unnecessary-type-assertion
?
Interesting. We haven't had a lot of users running without strictNullChecks
, and haven't had to think about this case before (that I know of). Similar with undefined as undefined
...
I poked a bit around it and I think it's because I'd enabled strict: true
in tsconfig.eslint.json
. That made typescript-eslint run with different type information than TypeScript proper. We in typescript-eslint generally don't accept bugs or feature requests for the mismatched-type-information situation because it's so rare for users to hit it.
If I turn off strictNullChecks
in tsconfig.eslint.json
then we end up with a bunch of no-unnecessary-type-assertion
complaints for code like id as NodePath<t.Identifier | null>
. Which, while technically correct, seems like a backwards step towards your goal in enabling strictNullChecks
.
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! I'm slightly leaning towards enabling this rule and ignoring edge cases, but both options are fine.
It seems this is not just happening with this PR. 🤔
https://github.com/babel/babel/pull/15681/files?diff=unified#diff-2584b855363a673ff1758cc21653fdbd86b1ad2ab7f1a851c2320999c9c6f84cR30
I had the same problem when I upgraded TS, so maybe it's not a tsconfig.eslint.json
specific issue.
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.
Perhaps we can temporarily use
eslint-disable-next-line
withstrictNullChecks: false
? I remember @JLHwung has astrictNullChecks: true
branch, but it was blocked by ts bug at the time, and it may be outdated now.(I'm willing to give this a try too, if that branch is outdated) By the way, cannull as null
be a special case ofno-unnecessary-type-assertion
?
Thanks for the reminder. I have rebased that branch but I still hits OOM: https://github.com/JLHwung/babel/tree/strict-null-checks, using TS 5.2.0 beta.
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.
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.
Good catch. Updated to 5.1.6 and it works for me now.
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.
#15716 (comment)
Special thanks for your detailed explanation of them, I can refer to this to enable more rules in the future!
This is a bit strange, OOM occurs on my local, can other reviewers try it? |
It doesn't oom for me (on macos), but maybe we could make |
However, I get warnings 🤔
|
This may be a bug, even if I use |
I was testing this locally again, and I realized that for some reason it's incredibly slow. To lint a single file, on `main` it takes ~4s, mostly spent while parsing.
while with this PR parsing takes 28s
I also tried rebasing this PR on top of
Btw @JLHwung it looks like with the new flat config we are linting polyfill dist files:
and
|
edc781b
to
9de7dff
Compare
@nicolo-ribaudo Yeah previously |
Oh right, I forgot about it. I have no idea 😬 I'll just delete it. Probably I at some point had to build an old commit and it generated that folder, and we have now no build script that deletes it since it's not expected to exist anymore. |
Otherwise TS will build a watch program for every *.js file including our own test fixtures, which overwhelms the memory
tsconfig.eslint.json
Outdated
{ | ||
"extends": "./tsconfig.json", | ||
"compilerOptions": { | ||
"allowJs": 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.
Is the allowJs
option necessary? I don't see more linting errors when I disable this option. Plus Babel has migrated to TS only so it is rare that we will ever import a .js from our source file.
I would like to minimize this config and hopefully in the future we can merge tsconfig.eslint.json
back to tsconfig.json
so we have consistent tsconfig between type checking and linting.
@nicolo-ribaudo The performance issue is fixed. Previously we didn't specify the including paths in @liuxingbaoyu This change should probably also fixes the OOM issue you noticed before, please do check. |
Amazing! Now it works great! Since I have |
64240f9
to
78c1e24
Compare
🙌 glad it's all working! I have to admit, I've lost track of what I can resolve and/or is directed at Babel folks - so I'm going to step back from updating this PR. Please do @ ping me if there's anything you want my action and/or opinion on though! I'm not unsubscribing, as I'm excited to watch things happen either way. 😄 |
Thanks! I go ahead and turn on the import _pluginCorejs2 from "babel-plugin-polyfill-corejs2";
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
const pluginCorejs2 = (_pluginCorejs2.default ||
_pluginCorejs2) as typeof _pluginCorejs2.default; as we are trying to support both cjs/esm build. I think this is fair and I ignore it. Side noteWhen I try to turn on the
when linting this file: https://github.com/babel/babel/blob/main/packages/babel-helper-create-class-features-plugin/src/features.ts. The error is thrown at if (type.types.some(t => tsutils.isTypeFlagSet(t, ignorableFlags))) {
return;
} I assume this is a bug in the rule and cc @JoshuaKGoldberg if you are interested. |
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.
@JoshuaKGoldberg Awesome work. Thank you.
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.
Awesome, thanks!
typescript-eslint@v6
with reworked configs
Yay! Thanks for collaborating on this everyone, really happy to see this PR land! ❤️🔥 |
Coming over from typescript-eslint/typescript-eslint#6760 & typescript-eslint/typescript-eslint#6760 (comment): this updates
@typescript-eslint/*
to the latest v6 alpha. You can read typescript-eslint.io/blog/announcing-typescript-eslint-v6-beta#user-facing-breaking-changes for the rationale behind the config changes. Essentially, the new recommended starter configs are:"plugin:@typescript-eslint/recommended-type-checked"
"plugin:@typescript-eslint/stylistic-type-checked"
I'm unfamiliar with the Babel codebase & its preferences so I disabled any rule that had more than one clear obvious resolution. But for each of the Todo: rules, I'd recommend reading the linked docs & deciding whether you'd want to enable it -- either in this PR (if the changes are straightforward) or in a followup.