Thanks for the PR,@Frezc!

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 onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day.

Codecov Report

Merging#2751 (b631e7a) intomaster (85c2720) willdecrease coverage by0.07%.
The diff coverage is92.01%.

@@            Coverage Diff             @@##           master    #2751      +/-   ##==========================================- Coverage   92.76%   92.68%   -0.08%==========================================  Files         310      312       +2       Lines       10393    10584     +191       Branches     2943     3004      +61     ==========================================+ Hits         9641     9810     +169- Misses        347      353       +6- Partials      405      421      +16

Flags with carried forward coverage won't be shown.Click here to find out more.

requiring type information in the rule is a breaking change.
We should not be making this change as it hugely limits the usability of the rule.

I setallowWithoutFullTypeInformation = true togetParserServices. This won't make tsconfig optional ?

This is the wrong approach to fix this - this should be fixed in the scope analyser - not in the rule.
Fixing this in the scope analyser would benefitall consumers of scope information, and not just this specific rule.

emm.. I have no idea what is scope analyser. Can you give me a brief explanation?

Thescope analyser is the tooling which runs over the AST and figures out what's a variable and where it is used.

Right now it doesn't understand that the decorators can consume values whenemitDecoratorMetadata is turned on.

So when it analyses

constructor(privatereadonlyconfig:ConfigService,privatereadonlyusersService:UsersService,privatereadonlylogger:Logger,privatereadonlyauthService:AuthService,  @InjectRepository(UserSessionRepository)privatereadonlyuserSessionRepository:UserSessionRepository,)

It doesn't understand thatprivate readonly config: ConfigService creates animplicit value reference onConfigService because there is a decorator andemitDecoratorMetadata is turned on.

Because it doesn't understand this - it just creates a type-only reference toConfigService.
This data then flows through to the lint rules (i.e.consistent-type-imports) which then look atimport ConfigService from './ConfigService', and sees only type-only imports - and flags it.

Fix the source of the data (the scope analyser) and the rule will "just work".

I mention this as there is one case that you haven't yet handled with your implementation code:

importThingfrom'thing';@decoratorclassFoo{constructor(arg:Thing,){}}

In this example, the decorator on the class with a constructor creates a similar reference to that of a decorated method.

This case at:https://github.com/typescript-eslint/typescript-eslint/pull/2751/files#diff-0cfd7f7e671424ab7fca326415a261843e64acdd13ecb12fc5133bd98b642948R184-R198

One other case I don't see tested, is this:

classFoo{method(    @decoratorarg1:Thing1,arg2:Thing2,){}}

repl

In this instance - TS will emit metadata aboutbothThing1 andThing2, even though onlyarg1 is decorated.

I missed this, thanks for hint.

One thing I didn't mention - please add some snapshot tests!

Your tests so far are great as they are explicit assertions of how it works, but as you've probably found out they're super cumbersome to write.

A snapshot test is less strict/safe as you have to manually eyeball the output, but they're nice because they're so easy to write. It means you can create isolated cases for each case and have a granular regression test.

They're also nice because they create a visual representation of the scope, which I found invaluable in figuring out how it all works.

https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/scope-manager/tests/fixtures/decorators

You can add the new scope manager optionhere

And then add this to the top of your fixture to turn on the option (example).

////@emitDecoratorMetadata = trueclassFixture{...}

I think this change broke@typescript-eslint/no-unused-vars rule for me.

After updating@typescript-eslint packages fromv4.13.0 tov4.14.0 the@typescript-eslint/no-unused-vars rule started showing false positive in my codebase. After explicitly disablingemitDecoratorMetadata in tsconfig.json the warning disappears.

See attached screenshot:

1:10  warning  'ChangeDetectorRef' is defined but never used                      @typescript-eslint/no-unused-vars

edit: There were related issues created#2946#2947

And as linked above, this was fixed in#2943

Seems something is still wrong with this change. After updating from 4.13.0 to 4.14.1 (or 4.14.0)@typescript-eslint/no-unused-vars reports false positives.

Single usage of importedMultiFactorState is marked with an arrow.

Don't comment on old, closed PRs. The comments are not easily discoverable by other users, and PRs do not show up in the "issues" tab - further hiding them from other users.

There is a pinned issue to follow, which is easily discoverable by using theissue search forno-unused-vars