typestack / class-validator

Decorator-based property validation for classes.
MIT License
10.86k stars 785 forks source link

question: Type safe property decorators #1929

Open karlismelderis-mckinsey opened 1 year ago

karlismelderis-mckinsey commented 1 year ago

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?

braaar commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

nicolaschambrier commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.