sikanhe / gqtx

Code-first Typescript GraphQL Server without codegen or metaprogramming
458 stars 13 forks source link

Resolve function is necessary when a built-in scalar can be undefined #63

Closed Emilios1995 closed 2 years ago

Emilios1995 commented 2 years ago

In the following code,

type SearchCriteria = {
  minPrice?: number,
  maxPrice?: number,
}

const SearchCriteria = t.objectType<SearchCriteria>({
  name: "SearchCriteria",
  fields: () => [
    t.field({ name: 'minPrice', type: t.Int , resolve: ({minPrice}) => minPrice }),
    t.field({ name: 'maxPrice', type: t.Int , resolve: ({maxPrice}) => maxPrice }),
  ]
})

The compiler throws an error if I don't provide the resolve function. The reason is that t.Int is of type Scalar<number | null>, while my SearchCriteria.minPrice is of type number | undefined, so the later doesn't extend the former.

It would be great if the built in scalars (if they're not non-nullable) supported the undefined value too, like so Int: Scalar<number | null | undefined>.

sikanhe commented 2 years ago

GraphQL does not understand undefined. You want resolve: ({minPrice}) => minPrice ?? null

I get that its a little inconvenient though. I actually suggest you changing the type definition of SearchCriteria to use number | null

Emilios1995 commented 2 years ago

GraphQL does not understand undefined. You want resolve: ({minPrice}) => minPrice ?? null

I get that its a little inconvenient though. I actually suggest you changing the type definition of SearchCriteria to use number | null

Thanks for answering. I understand that absent fields have to be represented as null rather than undefined because otherwise they would be missing from the serialized JSON, but I was under the impression that graphql-js will just convert undefined values to null—isn't that the case?

n1ru4l commented 2 years ago

@Emilios1995 Actually you are right: https://stackblitz.com/edit/graphql-pkmsvz?file=index.js

sikanhe commented 2 years ago

Mhmm good catch - maybe we should do this

sikanhe commented 2 years ago

PR to fix this in https://github.com/sikanhe/gqtx/pull/65

sikanhe commented 2 years ago

@Emilios1995 the version gqtx@0.8.2-4c93b62b.0 contains this change is on npm, let me know if you can try it out

sikanhe commented 2 years ago

In master

Emilios1995 commented 2 years ago

Thanks a lot!