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

feat(core): add generic type for transformed value in decorators factory #12276

Conversation

quangtran88
Copy link
Contributor

@quangtran88 quangtran88 commented Aug 24, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Currently, the return type of Reflector.get(ReflectableDecorator) resolves incorrectly if the transform option is used with the Reflector.createDecorator call and the transformed value has a different type than the input.

Issue Number: #12264

What is the new behavior?

Add a second optional generic param on Reflector.createDecorator to describe the transformed value type as following:

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


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

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@coveralls
Copy link

coveralls commented Aug 24, 2023

Pull Request Test Coverage Report for Build 40597bd8-8e2d-44be-8dd8-1c7b6891dc78

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 92.556%

Totals Coverage Status
Change from base Build 5bd17c50-f4ca-40e5-b4dd-2c0c2dd7c586: 0.04%
Covered Lines: 6428
Relevant Lines: 6945

💛 - Coveralls

@micalevisk
Copy link
Member

I didn't tested

Can we supply that second type arg without any transform function? Because we shouldn't, right

@quangtran88
Copy link
Contributor Author

quangtran88 commented Aug 24, 2023

I didn't tested

Can we supply that second type arg without any transform function? Because we shouldn't, right

Totally makes sense, I will make the transform option required if the second type arg is provided

@quangtran88 quangtran88 force-pushed the improve-reflector-decorator-transform-type branch 2 times, most recently from 0de5e03 to 7944f6e Compare August 24, 2023 04:16
@quangtran88 quangtran88 force-pushed the improve-reflector-decorator-transform-type branch from 7944f6e to cb6cc84 Compare August 27, 2023 07:02
@quangtran88 quangtran88 force-pushed the improve-reflector-decorator-transform-type branch from cb6cc84 to 65ba126 Compare August 27, 2023 07:06
@kamilmysliwiec kamilmysliwiec merged commit 974f2c6 into nestjs:master Aug 28, 2023
5 checks passed
@kamilmysliwiec
Copy link
Member

lgtm 🚀

@BrianJacobo
Copy link

LGTM! 🚀

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

Successfully merging this pull request may close these issues.

None yet

5 participants