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

0.17.0 relation _count type errors #204

Closed
JClackett opened this issue Nov 18, 2021 · 16 comments
Closed

0.17.0 relation _count type errors #204

JClackett opened this issue Nov 18, 2021 · 16 comments
Labels
enhancement New feature or request

Comments

@JClackett
Copy link

Describe the Bug
With the change in 0.17.0 regarding the _count field, all my fluent api relations types are now giving type errors

import { Team, User } from "@generated"

  @FieldResolver(() => Team, { nullable: true })
  async team(@Root() user: User): Promise<Team | null> {
    if (!user.teamId) return null
    return await prisma.user.findUnique({ where: { id: user.id }, }).team()
  }
Type 'import("<path>/node_modules/.prisma/client/index").Team | null' is not assignable to type 
'import("/<path>/node_modules/@generated/models/Team").Team | null'.
  Property '_count' is missing in type 
'import("/<path>/node_modules/.prisma/client/index").Team' but required in type 
'import("/<path>/node_modules/@generated/models/Team").Team'.ts(2322)

Team.d.ts(38, 5): '_count' is declared here.

In your release notes you mention that the _count property is now non-nullable, but isn't this just for the case when you pass in the option to select the _count property i.e

include: { _count: { select: { posts: true } } },

Related prisma issue

Basically, there's now a mis match between what prisma is providing and what typegraphql-prisma is saying will exist, typegraphql-prisma is now always expecting a _count property, but I'm pretty sure this isn't true?

@JClackett
Copy link
Author

You can see from their tests that they are expecting there to be no _count property when the include option isn't added

prisma/prisma@ce6e3d7

@MichalLytek
Copy link
Owner

_count always return 0, even if columns are null or no records found. That's why the _count GraphQL field is now non-nullable, because Prisma will always return some value.

It only applies to the generated resolvers of course. _count feature does not work when you have custom resolver with a Prisma chain returned.

There are known issues with Prisma and GraphQL types interoperability - see #107.

@JClackett
Copy link
Author

Dammm that's a shame, guess I have to either not type the return function or omit the _count property everywhere

@MichalLytek
Copy link
Owner

In order to avoid separate Prisma call, the _count field is working by reading GraphQL AST to check if the field is requested and then adding include to the main .findMany() call. You would need to do the same or write a field resolver that will use the value or call Prisma again (which might be tricky because you don't know the args then).

@JClackett
Copy link
Author

JClackett commented Nov 21, 2021

Have resorted to creating my own model classes for now, not ideal but can still use all the generated inputs/args etc

Happy to close this issue as it's basically a dupe of #107

@MichalLytek
Copy link
Owner

omit the _count property everywhere

export type XYZType = Omit<XYZ, "_count">;

@MichalLytek MichalLytek added the duplicate This issue or pull request already exists label Nov 22, 2021
@JClackett
Copy link
Author

Hmm yeah, but also need an actual class right? to use in the return types for the resolvers?

Example below

// user.model.ts
import {  User as TUser } from "@generated"
export type User = Omit<TUser, "_count">

// user.resolver.ts

// FIND USER
@UseAuth([Role.SUPER_ADMIN])
@Query(() => User, { nullable: true }) // ERROR: 'User' only refers to a type, but is being used as a value here.
async user(@Args() args: FindFirstUserArgs): Promise<User | null> {
  return await prisma.user.findFirst(args)
}

So I would have to create a class that extends the generated User, but then that errors..

export class User extends TUser {
  _count: undefined // Property '_count' in type 'User' is not assignable to the same property in base type 'User'.
}

Not sure if I'm doing something stupid, but can't seem to make TS happy

@MichalLytek
Copy link
Owner

I think we would need a generator option to not emit _count at all.

Prisma approach does not play well with GraphQL, especially with the custom resolvers.

@JClackett
Copy link
Author

Yeahhh, that would be great haha :)

I do feel like the default exported model should be _count: undefined, and for all the generated resolvers you can use an internal non-exported model with _count: UserCount. This means the generated resolvers would still work, and if I wanted to add _count to my User type I could manually extend it.

Would this work?

@MichalLytek
Copy link
Owner

No, we need to use the same class, otherwise it would end up with duplicated names and types being not interoperable.

So users will need to choose:

  • enable _count feature, use it in generated resolvers + apply the include: { _count manually in custom field resolvers
  • disable _count feature, no support for that in generated resolvers, you can always extend the base type and add support for _count manually

@MichalLytek MichalLytek reopened this Nov 24, 2021
@MichalLytek MichalLytek added enhancement New feature or request and removed duplicate This issue or pull request already exists labels Nov 24, 2021
@JClackett
Copy link
Author

Yep! Gotcha

Thanks for explaining!

@MichalLytek
Copy link
Owner

MichalLytek commented Nov 24, 2021

Forgot about the third option 🤦 Nullable _count so it will gracefully fail on custom resolvers if someone just don't care 😛

It should be the default option, as it worked like that before the "upgrade".

@dan2kx
Copy link

dan2kx commented Dec 1, 2021

I'm also facing this issue and have had to rollback to 0.16.6 to avoid having to fix a lot of issues with type inconsistencies, are you saying you will be making the count field generate like _count?: UserCount | null; or are you proposing a different solution?

@dickfickling
Copy link

looks like @MichalLytek fixed this (❤️) in 0.18.2 if you want to check it out @JClackett

@JClackett
Copy link
Author

Yep! All sorted! but in our team we actually decided to still create the custom model files anyway. I have some concerns with security when using the generated models as it enables the fields by default. Meaning the developer has to remember to add any security checks (I know typegraphql-prisma provides a way to add the checks). Using a custom model means the developer has to actively select which fields they want to expose. Less error prone in our opinion :)

@p0thi
Copy link

p0thi commented Sep 25, 2024

No, we need to use the same class, otherwise it would end up with duplicated names and types being not interoperable.

So users will need to choose:

  • enable _count feature, use it in generated resolvers + apply the include: { _count manually in custom field resolvers
  • disable _count feature, no support for that in generated resolvers, you can always extend the base type and add support for _count manually

Hi @MichalLytek , is there now a generator option to disable the _count field entirely in the generated models for custom resolvers?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants