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

Typed decorators - enhanced type-safety #221

Open
MichalLytek opened this issue Jan 3, 2019 · 5 comments
Open

Typed decorators - enhanced type-safety #221

MichalLytek opened this issue Jan 3, 2019 · 5 comments
Assignees
Labels
Discussion 💬 Brainstorm about the idea Enhancement 🆕 New feature or request
Milestone

Comments

@MichalLytek
Copy link
Owner

Right now it's dangerously easy to have inconsistence between schema type and TS type in class definition:

class Test {
  @Field(() => [Author])  // wrong item type
  actors: Actor[];

  @Field({ nullable: true })
  optionalField: string; // optional in schema, required in TS
}

It can be easily fixed (I was not aware of this possibility in TS) by using generics instead of predefined types like PropertyDecorator.

Quick demonstration of the proof of concept:

function Field<T>(typeFn: () => T) {
  return <TPropertyName extends string>(
    prototype: { [P in TPropertyName]: GetSimpleType<T> },
    propertyName: TPropertyName,
  ) => {
    console.log("TypeGraphQL FTW!");
  };
}

type GetSimpleType<T> = T extends BooleanConstructor ? boolean : never;

class Test {
  @Field(() => Boolean)
  okField: boolean;

  @Field(() => Boolean)
  badField: string;
}

image

Argument of type 'Test' is not assignable to parameter of type '{ badField: boolean; }'.
  Types of property 'badField' are incompatible.
    Type 'string' is not assignable to type 'boolean'

TODO: figure out how to get rid of false negative, like using custom scalars, etc.

Disclaimer: Inspired by @stephentuso ts-graphql 😃

@MichalLytek MichalLytek added Enhancement 🆕 New feature or request Discussion 💬 Brainstorm about the idea labels Jan 3, 2019
@MichalLytek MichalLytek added this to the 2.0.0 release milestone Jan 3, 2019
@MichalLytek MichalLytek self-assigned this Jan 3, 2019
@Veetaha
Copy link

Veetaha commented Mar 13, 2019

@19majkel94
Very good feature, but is there a way to provide strong typing support for method parameters decorators? I can't make tsc to issue an error for this code:

export interface ClassType extends Function {
    new (...args: any[]): unknown;
}

export type StaticMethodParameterDecorator = (
    <
        TMethodName  extends string | symbol,
        TClassType   extends ClassType & Record<TMethodName, (this: TClassType, ...args: unknown[]) => unknown>
    >
    (
        targetClass:    TClassType, 
        methodName:     TMethodName, 
        parameterIndex: number
    ) => void
);

function staticParameterDecorator(): StaticMethodParameterDecorator {
    return () => {};
}

class Cl {
    method(            // instance method, but no error
        @staticParameterDecorator()
        bool: boolean
    ) {
        return bool;
    }
}

Did you find a solution?

@MichalLytek
Copy link
Owner Author

Static method? Does TypeGraphQL supports this? 😜

You can check out the ts-graphql source code for help:
https://github.com/stephentuso/ts-graphql/blob/dev/src/decorators/Field.ts

@Veetaha
Copy link

Veetaha commented Mar 13, 2019

@19majkel94 Well, ok, we can skip this part, as there are no decorators targeted to static accessors/properties/methods in typegraphql. I inspected some of ts-graphql code previously, but failed to find a solution for strongly typing parameter decorators, that you will face when implementing yours if haven't yet.

@MichalLytek
Copy link
Owner Author

I think that typed decorators will be superseded by TypeScript transform plugin to enhance reflection system - more info soon 💪

@adhamsalama
Copy link

Hello @MichalLytek.
I hope all is well.
I have just found out about this behavior while using NestJS, and it led me to this GitHub issue, so, I am wondering if there are/will be any updates/solutions to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion 💬 Brainstorm about the idea Enhancement 🆕 New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants
@MichalLytek @Veetaha @adhamsalama and others