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

question: Type safe property decorators #1929

Open
karlismelderis-mckinsey opened this issue Jan 27, 2023 · 6 comments
Open

question: Type safe property decorators #1929

karlismelderis-mckinsey opened this issue Jan 27, 2023 · 6 comments
Labels
type: question Questions about the usage of the library.

Comments

@karlismelderis-mckinsey

I spotted that Typescript is not protecting against wrongly applied Property decorators.

export class Dto {
  @IsNumber() // <<<< No error here
  index: boolean;
}

After some googling I came up with quick example how to overcome it.

declare type NumberPropertyDecorator = <T, K extends keyof T>(target: [T[K]] extends [number] ? T : never, propertyKey: K) => void;

export declare function IsNumber(options?: IsNumberOptions, validationOptions?: ValidationOptions): NumberPropertyDecorator;

Do you think it's possible to extend this example and make Property Decorators more type safe?

@karlismelderis-mckinsey karlismelderis-mckinsey added the type: question Questions about the usage of the library. label Jan 27, 2023
@braaar
Copy link
Member

braaar commented Feb 1, 2023

This is quite cool! I can't think of any obvious pitfalls right now, and this seems like a very nice QoL feature. Has this been discussed previously, @NoNameProvided ?

@Dassderdie
Copy link

This solution doesn't work for decorated private and protected properties, as they are not in keyof T.

export class Dto {
  @IsNumber() // Argument of type 'Dto' is not assignable to parameter of type 'never'.ts(2345)
  private index: number;
}

As a solution, I would propose to solely check public properties:

declare type NumberPropertyDecorator = <
    T,
    K extends string | number | symbol
  >(
    // Private and protected properties are not in `keyof T`, therefore we are unable to type-check them
    target: K extends keyof T
        ? T[K] extends number
            ? T
            : never
        : unknown,
    propertyKey: K
) => void;

Note:
I also replaced [T[K]] extends [number] with T[K] extends number as I wasn't able to see the reasoning behind the additional wrapping tuple.

@karlismelderis-mckinsey
Copy link
Author

I'm happy to pick this up if we can agree that it make sense and @Dassderdie proposal is valid

@nicolaschambrier
Copy link

Validating only public properties would be totally acceptable, there's not much sense validating privates anyway imho.
This work would greatly help me (related to #1951, I didn't see this issue first).
I'm not sure how to make it work with IsOptional but pretty sure it's as much possible as validating the type.

Also it would be totally broken by pending features like #689.

Have to make sure this feature is actually expected and will be accepted because I feel like it's some work though...

@nberlette
Copy link

TypeScript 5.0 was finally released a couple weeks ago, and one of the biggest new features it brings is a great implementation of the Stage 3 Decorator Proposal. I highly recommend making the switch from the legacy style - assuming it's a realistic scenario for your project.

Along with a bunch of runtime improvements, the new API comes very well typed right out of the box. Check out the lib.decorators.d.ts file to see what I mean.

So far the only downside I've found is there is currently no support for parameter decorators. I'm not sure if that's in the roadmap for either the TypeScript or TC39 teams, but I'm fine with sacrificing that feature as the cost of upgrading.

@braaar
Copy link
Member

braaar commented Apr 11, 2023

So far the only downside I've found is there is currently no support for parameter decorators. I'm not sure if that's in the roadmap for either the TypeScript or TC39 teams, but I'm fine with sacrificing that feature as the cost of upgrading.

Does that affect this project at all? Our decorators are used for class properties only, as far as I understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: question Questions about the usage of the library.
Development

No branches or pull requests

5 participants