typescript-eslint
typescript-eslint copied to clipboard
typescript-eslint
Configs: Make consistent-type-imports 'error' for the recommended config
Before You File a Proposal Please Confirm You Have Done The Following...
- [X] I have searched for related issues and found none that match my proposal.
- [X] I have read the FAQ and my problem is not listed.
Description
It seems to me that using the @typescript-eslint/consistent-type-imports rule to enforce the use of import type where possible:
- Is beneficial (this blog post by maintainer @JoshuaKGoldberg explains it well)
- Is easy (
eslint --fixjust does the thing you want automatically) - And has no meaningful drawbacks.
Given all of that, is there any reason not to add it to the recommended configs?
Impacted Configurations
recommended-type-checked recommended-type-checked-only strict-type-checked strict-type-checked-only
Additional Info
No response
The reason we can't really add this to the recommended set is that it fixes to an opinionated style. It does top-level type specifier by default but not everyone likes that style.
That being said it could be in the strict set - which is intentionally opinionated.
hmm, similar discussion to https://github.com/typescript-eslint/typescript-eslint/issues/8667
This would be big pain point for all Angular users. That rule thinks that injected values in service constructor can be marked as type imports. But that breaks everything. So please do not put it in recommended set 🙏
import { Injectable } from '@angular/core';
import { HEROES } from './mock-heroes';
import { Logger } from '../logger.service'; // <- rule changes this to type import
@Injectable({providedIn: 'root'})
export class HeroService {
constructor(private logger: Logger) { } // <- this can't be type import, `Logger` is injected here
getHeroes() {
this.logger.log('Getting heroes ...');
return HEROES;
}
}
@rubiesonthesky is that addressed by https://github.com/typescript-eslint/typescript-eslint/issues/8335 and https://typescript-eslint.io/blog/changes-to-consistent-type-imports-with-decorators or a separate problem?
@kirkwaiblinger I thought I was testing with the latest version, but seems that I was using 7.7.1. I'll test with the latest. Thanks for pointing that issue!
I'm trying to run just this rule in my code base, usually linting takes 40 seconds. But trying to get errors for this one rule takes over 10 mins 🙈 Probably because there is so many errors.
I think I need to check this with better time, but at least quickly I was not able to get it working. emitDecoratorMetadata is not set by default, so I think that rule fix does not work out of box for Angular app. You can see their recomended tsconfig here: https://angular.io/guide/typescript-configuration - I think in some older versions it was used, but they stopped using it some point.
And if someone wants to test, I think this example project could be used from this page https://angular.io/tutorial/tour-of-heroes/toh-pt4
For the sample application that this page describes, see the live example / download example.
I tried to add emitDecorationMetadata to .eslintrc.json but it may be that I'm doing this wrong and it does not work like this with non-flat config? Also, using ts version 4.9.5. That said, I think this sounds like separate issue and I don't want to spam this discussion with side note investigation :)
"parserOptions": {
"project": [
"tsconfig.app.json"
],
"emitDecoratorMetadata": true,
"experimentalDecorators": true
},
@rubiesonthesky see playground where it is working as expected and NOT reporting.
emitDecoratorMetadata is not set by default
Of course it isn't - because it is not on by default in TS itself. To turn it on by default would be incorrect.
Ultimately this is in fact a stylistic concern that does not change a thing in many codebases:
- if you transpile with TS then using type-qualifiers doesn't change anything.
- if you transpile with non-TS tools then this may or may not change anything (tools like SWC will do a good job of guessing and eliding imports only used in type locations).
Add to that the fact that you really need a fixer for this rule - and no matter what you choose that fixer is enforcing a stylistic choice. Which is why the fixer is configurable.
Add to that the fact that out-of-the-box any codebase using legacy decorators + emitDecoratorMetadata will get incorrect reports without additional config.
I don't think this is a good candidate for recommended. So I'm a strong -1 for this being there.
But definitely +1 it should be on in strict/stylistic.
I also don't think consistent-* rules should be in recommended because they are after all stylistic. I do think this is fine to be in strict though.
That all makes sense, I agree it should only be in strict/stylistic after all
I'm -1 on this being included in any preset config. There's no one single style that fits everyone.
A lot of web apps are in a framework like Vite where the import style doesn't generally matter functionally. For those, it really doesn't matter whether you use type or not. So asking for a slightly more verbose style that includes type really just makes the code less readable.
ACK that the rules are super useful in projects where the import style does matter - including typescript-eslint. But I don't see any functional benefit to enforcing this style for others.
So asking for a slightly more verbose style that includes type really just makes the code less readable.
STRONGLY disagree with that sentiment. We use TS as our build tool yet we use type-only imports.
It's a very useful syntax as it allows you to explicitly annotate things which makes code clearer. Especially for reviewers within github that don't see the entire file and/or don't have type information /intellisense.
Its the explicitness of declaring "this import will be elided" that adds clarity and predictability to the code.
There's no one single style that fits everyone.
I'm not sure I see that POV. The point of the stylistic ruleset is that we are giving users an opinionated set of rules that we recommend that may not fit everyone's preferences.
The rule could be considered a style, sure, but it is a strict style that enforces some invariants, unlocks better tooling and makes things easier. TBH I personally would say that the rule itself is a correctness rule and the only stylistic thing is the fixer style.
Its the explicitness of declaring "this import will be elided" that adds clarity and predictability to the code.
If you don't care about which imports will be elided, then that clarity and predictability generally don't mean much. In an app where everything gets bundled into optimized chunks for production, what tangible benefits does this give?
In an app where everything gets bundled into optimized chunks for production, what tangible benefits does this give?
Well most FE build pipelines now avoid TS for transpilation because its performance is not good at scale. So instead they look for alternatives that are not type-aware.
These tools can often guess at what to elide - but they can also get it wrong easily. But with type-only imports it's no longer ambiguous what is elided and the tools can be guaranteed to work correctly.
It also increases the predictability of the build which makes it easier to understand why things are/aren't included in the resulting bundle - which is really helpful when trying to optimise bundle sizes and understand bundle regressions.
At Canva we used TS as our transpiler for many years and they also have used consistent-type-imports for as long as it has existed - both dates long before I joined the company.
ACK that in projects where build predictability and knowing what gets elided are important, consistent-type-imports: "no-type-imports" is good and useful. My pushback is that I think there are many projects where those don't matter. For many projects, most files don't have side effects and they don't have build predicatability or runtime performance issues. So the net benefit of a stylistic rule like this is ... roughly zero. No real measurable impact on day-to-day editing, or debugging ease, or product quality.
The drawbacks of enabling consistent-type-imports to "no-type-imports" on those projects are:
- It results in overly verbose imports, taking up code space
- It adds conceptual overhead: developers need to do slightly more work to read the imports
If we had a stylistic split between the "starter" and "comprehensive" configs the way recommended and strict are split, I'd say this might make sense in the "comprehensive" version. But for a standard starter like stylistic, I think it doesn't pass the bar of "would this go against the desired pattern for a nontrivial percentage of projects that are set up well".
My pushback is that I think there are many projects where those don't matter. For many projects, most files don't have side effects and they don't have build predicatability or runtime performance issues. So the net benefit of a stylistic rule like this is ... roughly zero.
That's what I'm disagreeing with though.
As a counter point - you could say the same thing about no-unused-vars. "well the minifier will tree-shake the unused imports and variables. Even if it doesn't/there isn't one - the imports won't have side-effects and most of the variables won't either so there won't be runtime perf issues"
Yet no-unused-vars is in the recommended set.
You could say similar things about prefer-const "the predictability of using const provides zero value for most codebases and you could write all your code without it and have no bugs".
Yet it's in our eslint-recommended set (and thus our recommended set).
What I'm saying is that a best practice can exist even if some codebases don't see tangible value from the practice. Standardising the ecosystem can provide value for the majority even if the minority just goes along with the standard.
What Brad said. More syntactic marker, even if unnecessary, is generally a good thing as it enforces invariants and decreases the entropy of your code. It's the same reason why we enforce override.
Both of those rules and the keyword provide directly apparent benefits:
-
no-unused-vars: at least more succinct and readable code, and sometimes even finding excess code paths or constructs that can be removed -
prefer-as-const: at least more succinct and readable code -
override: catches issues with inheritance shenanigans that sometimes aren't easy to catch otherwise
OTOH, the benefits of consistent-type-imports are much less apparent than the drawbacks.
even if the minority just goes along with the standard
What we've seen is that if we include rules where the perceived benefits are much less than the drawbacks -even with great docs- then people will protest and develop an aversion to using us. It's rare that we can get the community into a "just goes along with the standard" situation. Even things like floating/safe promises have taken a lot of ... community encouragement. Our easiest successes have been enforcing a standard agreed upon by many (or at least many of the influential folks) in the community.
Anyway, if I'm being outvoted here, then I'm ok with taking a step back and trying it out. We'll end up flighting this in community repos and seeing how it feels on them.
than the drawbacks
But what are the drawbacks? It has an autofixer!
We can query sourcegraph to figure out if inline or top-level is the most prevelant style so we can set the default for the autofixer and then it's invisible for people - the upgrade would just be "run with --fix".
The same can't be said for no-unused-vars. For example in the Canva codebase it wasn't turned on for a long time and so when they turned it on with the defaults it reported thousands of errors. Most of the errors were on unused parameters. But still they had to change the recommended config because the default style didn't suit and it was too large a task to fix by hand.
no-unused-vars: ... sometimes even finding excess code paths or constructs that can be removed
A good minifier will do that automatically too!
prefer-as-const: at least more succinct and readable code
There's a non-trivial number of people that would disagree with you there :) but we recommend the rule all the same!
But what are the drawbacks? It has an autofixer! We can query sourcegraph to figure out if inline or top-level is the most prevelant style so we can set the default for the autofixer and then it's invisible for people - the upgrade would just be "run with
--fix".
Isn’t the drawback that you will break code in repos that use legacy decorators? Of course you can then undo the fix, but it will still be annoying when after update the code is broken. :)
Isn’t the drawback that you will break code in repos that use legacy decorators?
Only if you have the combination of:
- legacy decorators
- experimental metadata
- not using type-aware linting
- imported values used only type locations AND runtime dependency on those values being named in decorator metadata
Then yes, you would get incorrect errors by default and would need to take a moment to set the parser options to indicate to our tooling to ignore files with decorators.
Though note that the false positives would only be in the files with decorators and that use imported values only in a type location that is covered by decorator metadata. I.e. It's realistically a small, small fraction of overall reports in the codebase.
Not to be sassy but - @angular/cli has 2.7m dls. @typescript-eslint/eslint-plugin` has 26.3m.
So half of all angular repos would be ~5% of our users. Which is reasonably rare in the grand scheme of things!
I'm content, if those numbers are good enough for you :) I was mostly after that this breakage for some users is acknowledged.
We never reached consensus on this and v8 has already shipped. I now even more firmly than before believe that this wouldn't be received well in the recommended, strict, or stylistic configs - only in a kind of "stylistic-strict" combo if one were to exist.
Closing out, but if anybody wants to start a new GitHub Discussion on it, that would be a reasonable next step.
Thanks for the discussion everyone! ❤️