Configs: Make consistent-type-imports 'error' for the recommended config #9036
Comments
Sweater-Baron
added
package: eslint-plugin
|
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 #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 #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. 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
I tried to add emitDecorationMetadata to "parserOptions": {
"project": [
"tsconfig.app.json"
],
"emitDecoratorMetadata": true,
"experimentalDecorators": true
}, |
|
@rubiesonthesky see playground where it is working as expected and NOT reporting.
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:
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 + I don't think this is a good candidate for 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 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. |
STRONGLY disagree with that sentiment. 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.
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. |
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? |
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 |
|
ACK that in projects where build predictability and knowing what gets elided are important, The drawbacks of enabling
If we had a |
That's what I'm disagreeing with though. Yet You could say similar things about Yet it's in our 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 |
|
Both of those rules and the keyword provide directly apparent benefits:
OTOH, the benefits of
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. |
But what are the drawbacks? It has an autofixer! The same can't be said for
A good minifier will do that automatically too!
There's a non-trivial number of people that would disagree with you there :) but we recommend the rule all the same! |
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. :) |
Only if you have the combination of:
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. |
|
For me it sounds like half of Angular repos :D |
|
Not to be sassy but - 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. |
Before You File a Proposal Please Confirm You Have Done The Following...
Description
It seems to me that using the
@typescript-eslint/consistent-type-importsrule to enforce the use ofimport typewhere possible:eslint --fixjust does the thing you want automatically)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 text was updated successfully, but these errors were encountered: