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

Static value return type argument for decorators #514

Closed
Tybot204 opened this issue Jan 8, 2020 · 3 comments
Closed

Static value return type argument for decorators #514

Tybot204 opened this issue Jan 8, 2020 · 3 comments
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request Wontfix ❌ This will not be worked on

Comments

@Tybot204
Copy link

Tybot204 commented Jan 8, 2020

As a quick preface, sorry for the long issue. It took me a bit to realize what was actually happening as my issue so I wanted to be thorough!

Is your feature request related to a problem? Please describe.
This originally stems from an issue testing use of decorators. Jest code coverage (uses Istanbul) fails for any decorator used from type-graphql that takes a function as one of the arguments, since that function never actually gets called if unit testing the class functions directly.

Here are some example classes:

// This class would pass with 100% test coverage
@ObjectType()
export class User {
  @Field()
  public email!: string
}

// This class would also pass with 100%, since the argument given is not a function
@ObjectType()
export class User {
  @Field({ nullable: true })
  public email?: string
}

// This class would not have 100% test coverage, due to the function passed to @Field
@ObjectType()
export class User {
  @Field(() => String)
  public email!: string
}

I realize in the example above, this can simply be resolved by removing the return type function since type-graphql infers the value should be a string. However, this is not possible everywhere and causes more issues in places like resolvers, since a return type is required:

@Resolver()
export class UserResolver {
  // The following @Query line would not count as covered,
  // even if new UserResolver().user() was called in tests
  @Query(() => User)
  public async user() {
    return { email: "[email protected]" }
  }
}

Describe the solution you'd like
I propose adding an option to specify the return type directly as the first argument to directives instead of a return type function. I understand this argument is currently a function to provide documentation and readability in the first void argument. I don't think we should remove this option, but just add an additional type signature that allows a static type like so:

@ObjectType()
export class User {
  @Field(String) {
  public email!: string
}

@Resolver()
export class UserResolver {
  @Query(User)
  public async user() {
    return { email: "[email protected]" }
  }
}

I've started to dig into type-graphql source and would be happy to take a deeper look and open a PR if this feature is approved!

Describe alternatives you've considered
A temporary solution I've worked with is explicitly telling Istanbul to skip code coverage for decorators that I pass a return type function argument to. This works for now, but is going to become cumbersome to maintain. Other developers might miss adding this to the project I'm working on (and probably even myself)! This also adds bloat since every decorator now has a comment that is useless to devs reading the code.

I also found a potential solution here in Stackoverflow, but also don't particularly like it for similar reasons to above. It provides extra complexity and hinders development ease just for the sake of having 100% code coverage.

Additional context
#506 - This issue touched on this problem with code coverage, but reading it I don't believe the real cause of the missing code coverage was realized (that the argument to the decorator is specifically a function, not just any argument gets counted against code coverage).

kulshekhar/ts-jest#488 - TS-Jest actually does already ignore decorators by default, but this issue of code coverage persists since Jest only auto-ignores the call to the decorator, not the function being passed as an argument

@MichalLytek
Copy link
Owner

I propose adding an option to specify the return type directly as the first argument to directives instead of a return type function

Just because you unit test your code and complain about the right not full coverage?
Maybe it's time to switch to integration testing and execute the built schema that you use in production? It's not too hard to make that test setup as you can see in the tests of TypeGraphQL 😉

@MichalLytek MichalLytek added Community 👨‍👧 Something initiated by a community Discussion 💬 Brainstorm about the idea Enhancement 🆕 New feature or request labels Jan 8, 2020
@Tybot204
Copy link
Author

Tybot204 commented Jan 8, 2020

Just because you unit test your code and complain about the right not full coverage?

Yes. 😅

I don't think it's unreasonable to have unit test coverage accurately represent tests in addition to integration testing. Unit and integration tests aren't mutually exclusive in my situation!

@MichalLytek
Copy link
Owner

If you have at least one test case that actually builds the schema, then the decorator callback should be covered.

Closing as won't fix 🔒

@MichalLytek MichalLytek added Wontfix ❌ This will not be worked on and removed Discussion 💬 Brainstorm about the idea labels Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request Wontfix ❌ This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants