nullable inputs/args are not covered in the docs very well

[South-Paw]

Firstly, this feels a bit stupid to be noticing after using type-graphql for 2 years but here it goes...

Describe the issue

The docs seemingly made it clear to me that having nullable was equivalent to an optional type... e.g.

@Field({ nullable: true })
averageRating?: number;

and/or

@InputType()
export class RecipeInput {
  @Field()
  title: string;

  @Field({ nullable: true })
  description?: string;
}

and this is mostly true for fields but it comes to args/inputs its misleading ... as I've just found out while debugging an issue in my project.

The correct typescript types for this should be

@Field({ nullable: true })
averageRating?: number | null;

and/or

@InputType()
export class RecipeInput {
  @Field()
  title: string;

  @Field({ nullable: true })
  description?: string | null;
}

I haven't seen this mentioned in the docs (and it feels like I'm stating the obvious) but when using nullable, null is a valid input for args/fields/etc and I've just realized a bunch of code with optional chaining is very broken now 😆

Are you able to make a PR that fix this?

Would be good to get it confirmed that I'm not crazy in noticing this and it's not already mentioned somewhere I haven't seen... its a bit late where I am and I've also been chasing this issue all afternoon only to realize this now... 😭

Additional context

Specifically, I've been working on Query with the following signature...

@Query(() => SomeResponse)
async allThings(
  @Arg('filter', { nullable: true }) filter?: SomeFilter,
): Promise<Response<ISomething>> {
  // ...

  console.log(filter?.search); // undefined or SomeFilter right?
}

and just realized this is a very valid GQL query for it:

{
  allThings(filter: null) {
    id
  }
}

and with this, console.log(filter?.search) will now error because "Cannot read properties of null (reading 'search')"

[MichalLytek]

It's just a trade-off over simplicity vs. correctness.

In runtime, for input you might get undefined (field not provided) or null (value set to null explicitly).

However, declaring union type Foo | null breaks TypeScript reflection system.
So you then have to do:

@Field(type => Float, { nullable: true })
averageRating?: number | null;

That's why in docs it is made simpler:

@Field({ nullable: true })
averageRating?: number;

Because all the if (foo) and foo?.bar checks works the same for null and undefined in JS.
So most of the time there's no need to distinguish them 😉

[South-Paw]

@MichalLytek thanks for the response - totally agree its a trade off but becomes really important when doing things like updates where null can mean clearing a field vs undefined is no change (this is where I have been really badly bitten now!)

I think we need a section in the docs that covers this clearly - because I haven't noticed this in the 2 years I've heavily used type-graphql (honestly very surprised I haven't noticed it earlier either tbh)...

[South-Paw]

additional to previous comment; it makes sense to simplify this for Fields of an ObjectType, but for InputType and Args it is important to distinguish that null is a totally valid input when fields/args are marked nullable: true - and this specifically isn't mentioned anywhere in the docs as far as I can see?

[MichalLytek]

for InputType and Args it is important to distinguish that null is a totally valid input when fields/args are marked nullable: true - and this specifically isn't mentioned anywhere in the docs as far as I can see?

That's possible, try to find if it's described somewhere and make a PR with the changes over that "nullable" topic 😉

[South-Paw]

for InputType and Args it is important to distinguish that null is a totally valid input when fields/args are marked nullable: true - and this specifically isn't mentioned anywhere in the docs as far as I can see?

That's possible, try to find if it's described somewhere and make a PR with the changes over that "nullable" topic 😉

Would be happy to make that contribution if that's ok.

But first I need some sleep and then I've got a few args and input types that urgently need fixing up in a project to handle nulls! 😭

(updated link in issue with example of a input type from the validation section of docs)

[South-Paw]

@MichalLytek 👋 I thought I'd add some additional bits I've subsequently found...

The following is an example of why the optional chaining wasn't working as expected. In my previous example everything would be fine - but this is a better representation of what was happening in my case:

// SomeResolver.ts
@Query(() => SomeResponse)
async allThings(
  @Arg('filter', { nullable: true }) filter?: SomeFilter,
): Promise<Response<ISomething>> {
  this.someService.foo(filter);
}


// SomeService.ts
foo(filter?: Filter = {}) {
  console.log(filter?.search); // `{}` or `Filter` right?
}

But because I previously didn't realize null could and would flow through, this appears to be fine.

The types are all good, there's no issues on the surface until you call the GraphQL server with:

{
  allThings(filter: null) {
    id
  }
}

and voila; Cannot read properties of null (reading 'search') because filter can be null at runtime.

---

I have also been working through my types and resolvers updating things that should allow null to allow null and restricting those that don't. I have been using class-validator's @NotEquals(null) to restrict input types however for those that should allow null as a value, I have been forced into doing the following...

// SomeResolver.ts
@Query(() => SomeResponse)
async allThings(
  @Arg('filter', () => SomeFilter || null, { nullable: true }) filter?: SomeFilter | null,
): Promise<Response<ISomething>> {
  this.someService.foo(filter)
}

Without specifying the return type of () => SomeFilter || null, type-graphql throws errors like:

Unable to infer GraphQL type from TypeScript reflection system. You need to provide explicit type for argument named 'filter' of 'allThings' of 'SomeResolver' class.

This is also the case for InputTypes:

@InputType()
export class UpdatePayload {
  // `id` is required when updating
  @Field()
  id!: string;

  // `name` can either be undefined if unchanged or string when changing
  @Field({ nullable: true })
  @NotEquals(null)
  name?: string;

  // `description` can be one of undefined if unchanged, string when changing or null when clearing
  @Field(() => String || null, { nullable: true })
  description?: string | null;
}

This also requires the buildSchema option of validate: { skipMissingProperties: false }, to be set otherwise validation is skipped on null values.

This is a bit more than I was expecting to have to do for this and feels like I'm uncovering a bit more of a gap here? 🤔

• Is there a better way to do these that you can see/think of?
• Should type-graphql be better at handling/infering things with a union of null?

Thanks

[MichalLytek]

() => SomeFilter || null

This is a runtime expression, JS value that is evaluated, just like 2 + 2. It's not a type declaration, you can provide there only the single value. That's why we use { nullable: true } not || null.

Should type-graphql be better at handling/infering things with a union of null?

This is a TypeScript limitation, I can't do anything about it.

[South-Paw]

This is a TypeScript limitation, I can't do anything about it.

No worries, I figured it was something like that - thank you for taking the time to read & respond! 👍

[revskill10]

Lol, spent whole morning due to just lacking of an example for explicit type
For anyone just read the docs and get the error like this one

Error: Unable to infer GraphQL type from TypeScript reflection system. You need to provide explicit type for argument named 'name' of 'createBook' of 'CategoryResolver' class.

@Mutation(type => Boolean)
createBook(        
    @Ctx() ctx: Context,
    @Arg("name", () => String, { nullable: true }) name?: String,
): Promise<boolean> {
    return ctx.productClient.sendCommand({ name: 'test' }).catch(() => false).then(() => true)
}

The pain point is, for Query you don't need explicit type, but for Mutation, it's required now.

[phuongvd-dipro]

Use @field() with @notequals(null) to value not undefined
@field()
@notequals(null)

[philipp-schmidt]

Sorry for the thread necromancy, but I wanted to add that class-validator actually has a skipUndefinedProperties.
This was discussed and merged here: typestack/class-validator#308

This also requires the buildSchema option of validate: { skipMissingProperties: false }, to be set otherwise validation is skipped on null values.

Using skipUndefinedProperties instead of skipMissingProperties will actually validate null properly.