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

strongly-typed decorators: infer return type of transform prop #12264

Closed
3 of 15 tasks
elijaholmos opened this issue Aug 22, 2023 · 5 comments
Closed
3 of 15 tasks

strongly-typed decorators: infer return type of transform prop #12264

elijaholmos opened this issue Aug 22, 2023 · 5 comments
Labels
needs triage This issue has not been looked into

Comments

@elijaholmos
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

The strongly-typed decorator factories introduced in #12237 are really cool. Thanks for adding them.

I noticed a small problem that comes up with a slightly advanced usage. Currently, the return type of Refector.get(ReflectableDecorator<T>) is typed as whatever ReflectableDecorator<T> was initialized as, using Reflector.createDecorator. This is great for simple metadata storage, but if the transform method is used to alter the input value before it is stored, the return type does not update to match the transform method's return type.

As an example:

// roles.decorator.ts
import { Reflector } from '@nestjs/core';

export const Roles = Reflector.createDecorator<string | string[]>({
  key: 'roles',
  transform: (...roles: string[]): string[] => roles ?? [],
});
// decorator usage
import { Roles } from './roles.decorator';
@Roles('admin')
// roles.guard.ts
import { CanActivate, ExecutionContext, Injectable } from '@nestjs/common';
import { Reflector } from '@nestjs/core';
import { Roles } from './roles.decorator';

@Injectable()
export class PolicyGuard implements CanActivate {
  constructor(protected readonly reflector: Reflector) {}

  canActivate(context: ExecutionContext): boolean {
    const roles = this.reflector.get(Roles, context.getHandler()) || [];

    return roles.some((role) => role === 'admin'); // TypeError: Property 'some' does not exist on type 'string | string[]'.
  }
}

Minimum reproduction code

https://stackblitz.com/edit/nestjs-typescript-starter-yoxyxj?file=src%2Froles.guard.ts

Steps to reproduce

No response

Expected behavior

Ideally, the typing system would be able to detect the return value on the transform property, and use that as the return value of the Refector.get(ReflectableDecorator<T>) method.

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

10.2.1

Packages versions

[System Information]
OS Version : Windows 10
NodeJS Version : v18.16.0
PNPM Version : 8.6.12

[Nest CLI]
Nest CLI Version : 10.1.12

[Nest Platform Information]
platform-express version : 10.2.1
mapped-types version : 2.0.2
schematics version : 10.0.2
passport version : 10.0.1
graphql version : 12.0.8
testing version : 10.2.1
apollo version : 12.0.7
common version : 10.2.1
config version : 3.0.0
core version : 10.2.1
jwt version : 10.1.0
cli version : 10.1.12

Node.js version

18.16.0

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

No response

@elijaholmos elijaholmos added the needs triage This issue has not been looked into label Aug 22, 2023
@micalevisk
Copy link
Member

micalevisk commented Aug 22, 2023

another thing that we could improve (probably not feasible):

transform: (...roles: string[]): string[] => roles ?? []

it should error out at compiler time because of
Reflector.createDecorator<string | string[]>

that roles parameter will be string[][] when using @Roles(['foo']). And it will be string[] when using @Roles('foo')

@quangtran88
Copy link
Contributor

quangtran88 commented Aug 23, 2023

I think we can add a second optional generic param on Reflector.createDecorator to describe the transformed value type as following:

const Roles = Reflector.createDecorator<string | string[], string[]>({
  key: 'roles',
  transform: (...roles: string[]): string[] => roles ?? [], // Must return type string[]
});


const roles = this.reflector.get(Roles, context.getHandler()) // Will resolve as type string[]

@micalevisk
Copy link
Member

micalevisk commented Aug 23, 2023

yeah, that should work. I just made a PoC

image

image

@quangtran88
Copy link
Contributor

I created a PR #12276 for this.

@kamilmysliwiec
Copy link
Member

Let's track this here #12276

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into
Projects
None yet
Development

No branches or pull requests

4 participants