-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[consistent-type-imports] add support for passive value usage by decorators when emitDecoratorMetadata
is enabled
#2559
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
Comments
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 |
Disclaimer: I know absolutely nothing about 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? 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. I think this should probably be fixed upstream in next js. |
@bradzacher It's Nest.js, not Next.js. And I agree its approach to Dependency injection is very odd |
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 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 😄 |
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? 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. |
Nest uses Typescripts There is a |
Oh wow. 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. 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:
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:
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 |
nest.js
's dependency injectionemitDecoratorMetadata
is enabled
Seehttps://github.com/typescript-eslint/issues/2559#issuecomment-692780580 for additional information
Seehttps://github.com/typescript-eslint/issues/2559#issuecomment-692780580 for additional information
Repro
Expected Result
I expected the result to work
Actual Result
It "fixes" the code to
This causes errors like
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
@typescript-eslint/eslint-plugin
4.1.0
@typescript-eslint/parser
4.1.0
TypeScript
4.0.2
ESLint
node
14
The text was updated successfully, but these errors were encountered: