[consistent-type-imports] add support for passive value usage by decorators when emitDecoratorMetadata is enabled

[vegerot]

• [x ] I have tried restarting my IDE and the issue persists.
• [ x] I have updated to the latest version of the packages.
• [x ] I have read the FAQ and my problem is not listed.

Repro

{
  "rules": {
    "@typescript-eslint/consistent-type-imports": 'error'
  }
}

import {
  Controller,
  Logger,
} from '@nestjs/common';
import { ConfigService } from './config/config.service';
import { UserSessionRepository } from '../entities/user-session/user.session.repository';
import { UsersService } from './users/users.service';
import { AuthService } from './auth/auth.service';

Expected Result
I expected the result to work

Actual Result
It "fixes" the code to

import {
  Controller,
} from '@nestjs/common';
import type { Logger } from '@nestjs/common';
import type { ConfigService } from './config/config.service';
import { UserSessionRepository } from '../entities/user-session/user.session.repository';
import type { UsersService } from './users/users.service';
import type { AuthService } from './auth/auth.service';

This causes errors like

Error: Nest can't resolve dependencies of the AppController (?, Function, Function, Function, UserSessionRepository). Please make sure that the argument Function at index [0] is available in the RootTestModule context.

Potential solutions:
- If Function is a provider, is it part of the current RootTestModule?
- If Function is exported from a separate @Module, is that module imported within RootTestModule?
  @Module({
    imports: [ /* the Module containing Function */ ]
  })

I want to say I know this is Nest's problem and not ESLint's. The linter is correctly identifying this import is only used in a type context. But for reasons I really dislike about Nest.js, they use this information in their dependency injection process.

This thread is not really to report a bug, but to ask for advice on how to proceed? Would there be the potential to add an option to consistent-type-imports to regex types not to fix (where I could put my Services)? Or should I just not use this rule in any Nest projects?

Versions

package	version
@typescript-eslint/eslint-plugin	4.1.0
@typescript-eslint/parser	4.1.0
TypeScript	4.0.2
ESLint	latest
node	14

[vegerot]

Workaround: I was able to work around this issue by running

sed -E -i "s|(import \{ .*Service \})|// eslint-disable-next-line @typescript-eslint/consistent-type-imports\n\1|g" src/**/*.ts

with a few other manual fixes

[bradzacher]

Disclaimer: I know absolutely nothing about nextjs nest.js. I don't know what it is, what it does, or how it works.

---

I don't quite understand why they can't resolve type only imports. Does nestjs have a build step that runs after the TS code is transpiled?
It seems pretty silly to rely on unused imports to do dependency injection.

I don't think having an option which enforces that some imports remain as value imports is a good idea for the rule as a whole.
The config for it would be weird and fragile depending on your setup, and it would create a really, really weird experience for end users.

I think this should probably be fixed upstream in next js.
That, or someone should fork the rule specifically for the nestjs usecase.

[vegerot]

@bradzacher It's Nest.js, not Next.js. And I agree its approach to Dependency injection is very odd

[jmcdo29]

Hi there, I'm one of the core team members with NestJS. From the view of how the dependency injection is handled, it doesn't make sense to enforce import type across all cases, especially as Nest uses the class's name to determine what will be injected when. When import type is used, that name and metadata get stripped from the code and all that is left is Function, meaning that there isn't a name for Nest to resolve.

I'm not quite sure what constitutes an "unused import", but it seems that it's the fact that the class is used for it's type and never instantiated by the dev. If that's the case, this rule probably should not be used by developers using the NestJS framework for the reason mentioned above.

I'm open to hearing more about what's possibly going on, and what can be done about it as well 😄

[bradzacher]

So does NestJS's build step happen before or after the TS transpilation?

If it happens after - how do you resolve the arguments to the constructor?

eg how is this:

import type { Logger } from '@nestjs/common';
import type { ConfigService } from './config/config.service';
import { UserSessionRepository } from '../entities/user-session/user.session.repository';
import type { UsersService } from './users/users.service';
import type { AuthService } from './auth/auth.service';

constructor(
  private readonly config: ConfigService,
  private readonly usersService: UsersService,
  private readonly logger: Logger,
  private readonly authService: AuthService,
  @InjectRepository(UserSessionRepository)
  private readonly userSessionRepository: UserSessionRepository,
)

any different to this:

import { Logger } from '@nestjs/common';
import { ConfigService } from './config/config.service';
import { UserSessionRepository } from '../entities/user-session/user.session.repository';
import { UsersService } from './users/users.service';
import { AuthService } from './auth/auth.service';

constructor(
  private readonly config: ConfigService,
  private readonly usersService: UsersService,
  private readonly logger: Logger,
  private readonly authService: AuthService,
  @InjectRepository(UserSessionRepository)
  private readonly userSessionRepository: UserSessionRepository,
)

or how is it different to this (pure JS with a decorator):

import { Logger } from '@nestjs/common';
import { ConfigService } from './config/config.service';
import { UserSessionRepository } from '../entities/user-session/user.session.repository';
import { UsersService } from './users/users.service';
import { AuthService } from './auth/auth.service';

constructor(
  config,
  usersService,
  logger,
  authService,
  @InjectRepository(UserSessionRepository)
  userSessionRepository,
)

Does NestJS's build step do some automatic name mapping here?
I.e. does it look at the name of the argument config and go: "look for Config variable, if not exists look for ConfigService"? Similar for usersService does it go "look for UsersService variable"?

If that is correct - then yeah, there's nothing that can be done. Our lint rule cannot handle the implicit references to variables that are created by NestJS's build step.

Someone would have to fork this lint rule for use with NestJS's specific setup.

[jmcdo29]

Nest uses Typescripts emitDecoratorMetadata with experimentalDecorators the Reflect system to read the parameters of the constructor properly. With pure JS, Babel decorators like @Bind() are necessary to ensure that the metadata is properly reflected and readable. All of this metadata reading happens at runtime, when await NestFactory.create() is ran, and nothing happens during compile time.

There is a nest build command available from the CLI, but that's mostly just a wrapper around tsc or webpack depending on the dev's settings and options passed.

[bradzacher]

Oh wow.
TIL TS will emit use the type information to emit additional metadata about parameters if there is a decorator.

constructor(
  private readonly config: ConfigService,
  private readonly usersService: UsersService,
  private readonly logger: Logger,
  private readonly authService: AuthService,
  @InjectRepository(UserSessionRepository)
  private readonly userSessionRepository: UserSessionRepository,
)

Will emit this additional block:

Foo = __decorate([
    __param(4, InjectRepository(UserSessionRepository)),
    __metadata("design:paramtypes", [
      typeof (_a = typeof ConfigService !== "undefined" && ConfigService) === "function" ? _a : Object,
      typeof (_b = typeof UsersService !== "undefined" && UsersService) === "function" ? _b : Object,
      typeof (_c = typeof Logger !== "undefined" && Logger) === "function" ? _c : Object,
      typeof (_d = typeof AuthService !== "undefined" && AuthService) === "function" ? _d : Object,
      typeof (_e = typeof UserSessionRepository !== "undefined" && UserSessionRepository) === "function" ? _e : Object,
    ]),
], Foo);

So this is how it works and why it breaks with a type-only import - there is an implicit reference here.

It's worth noting that if you do not have any decorators at all, then TS will not supply any metadata.

---

Hmmmmm. We could actually build support for this into the scope analyser.
If the user is using type-aware linting, we can inspect the tsconfig and determine if the emitDecoratorMetadata option is on.

Then we can follow the following rules to create an additional value reference for the types:

For a constructor, these two cases will cause TS to emit metadata about constructor parameter types:

1. If you have a decorator on any of the constructor parameters
2. If you have a decorator on the class itself

For a non-constructor, non getter/setter methods, TS will emit metadata about the method parameter types AND the method return type in these cases:

1. If you have a decorator on any of the method parameters
2. If you have a decorator on the method itself

For a class property, TS will emit metadata about the property type if and only if the property has a decorator.

For getters / setters, TS will emit metadata about the property type if and only if one of the methods is annotated with a decorator (it will completely ignore if you decorate a setter parameter).

---

This would be no small piece of work, but happy to accept a PR