Skip to content
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

Constructor parameter decorators should allow undefined as the type of key #10959

Closed
2 of 15 tasks
DanielRosenwasser opened this issue Jan 27, 2023 · 1 comment
Closed
2 of 15 tasks

Comments

@DanielRosenwasser
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

TypeScript 5.0 introduces some tightening on decorator type-checking. This is basically a bug fix, but it affects any decorator used on constructor parameters.

export declare const Inject:
  (entity: Function) =>
    (target: object, key: string | symbol, index?: number) => void;

export class Foo {}

export class C {
  constructor(@Inject(Foo) x: any) {}
}

Here, the decorator function returned by Inject says it takes a string | symbol for its key parameter; however, if the usage above is supposed to be valid, then this declaration always been incorrect. Constructor parameters always receive undefined for their key.

TypeScript 5.0 enforces checking on this, and users of existing decorator libraries like NestJS will receive an error like the following:

Unable to resolve signature of parameter decorator when called as an expression.
  Argument of type 'undefined' is not assignable to parameter of type 'string | symbol'

More details are available on the TypeScript repo at microsoft/TypeScript#52435

Minimum reproduction code

https://www.typescriptlang.org/play?ts=5.0.0-dev.20230126#code/JYWwDg9gTgLgBAbzgSQHYCsCmBjeBfOAMyghDgCIABVTAZxnVoHptSQJVyBuAWACh+2ADYBDWrTgBhRPzhy4rVPSgBXXNAAUlNFlwaAyjCjBUAcwCURCBABccZSYuI8-PEA

Steps to reproduce

Try any example of @Inject on a constructor parameter with typescript@next.

Expected behavior

Nest should update its declaration to allow undefined as a key type.

Package

  • I don't know. Or some 3rd-party package
  • @nestjs/common
  • @nestjs/core
  • @nestjs/microservices
  • @nestjs/platform-express
  • @nestjs/platform-fastify
  • @nestjs/platform-socket.io
  • @nestjs/platform-ws
  • @nestjs/testing
  • @nestjs/websockets
  • Other (see below)

Other package

No response

NestJS version

No response

Packages versions

9.2.1

Node.js version

No response

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

No response

@DanielRosenwasser DanielRosenwasser added the needs triage This issue has not been looked into label Jan 27, 2023
@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Jan 27, 2023

I just created a PR for this issue #10970

Also, on a side note, thank you (and the entire TypeScript team) for allowing us to keep using the original TS-decorators implementation in v5. Switching to the JS decorators (current proposal) would drastically degrade DX since it currently doesn't support param-based decorators + emitDecoratorMetadata. Both features are essential not only for the NestJS framework (and its ecosystem) but for some other, popular TS libraries too (be it typeorm, class-transformer, class-validator etc., each one having +1M downloads/week). I hope we can postpone the migration for the foreseeable future given how impactful (in a negative sense) it would be until there's a full feature parity between these two implementations.

@micalevisk micalevisk removed the needs triage This issue has not been looked into label Feb 1, 2023
willsoto pushed a commit to willsoto/nestjs-prometheus that referenced this issue Apr 7, 2023
I found this issue #1667 that had the same errors as me, but the comments didn't help. I also found a different issue nestjs/nest#10959, and the fix they used nestjs/nest#10970, and used that to fix the problem with the InjectMetric decorator.

The problem is that the decorator types are wrong. Record<string, unknown> fails with strict checks, and the key may also be undefined.

Fixes #1667
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants