skip/include directives do not avoidOptionals if field type is a list

[brainkim]

Describe the bug

The @skip and @include directives seem to be generating optional types for GraphQLList fields, even when avoidOptionals is true.

To Reproduce
Steps to reproduce the behavior:
https://codesandbox.io/s/practical-swanson-b93wg

1. My GraphQL schema:

type Query {
  me: User!
}

type User {
  messages: [Message!]!
}

type Message {
  content: String!
}

2. My GraphQL operations:

query MyQuery($include: Boolean!) {
  me {
    messages @include(if: $include) {
      content
    }
  }
}

3. My codegen.yml config file:

schema: schema.graphql
documents: document.graphql
generates:
  types.ts:
    plugins:
      - typescript
      - typescript-operations
config:
  avoidOptionals: true
  nonOptionalTypename: true

Expected behavior

I expect TypeScript optionals not to be included in the generated output.

Actual behavior

export type MyQueryQuery = (
  { __typename: 'Query' }
  & { me: (
    { __typename: 'User' }
    & { messages?: Maybe<(
      { __typename: 'Message' }
      & Pick<Message, 'content'>
    )> }
  ) }
);

messages is optional.

Environment:

• OS: MacOS Big Sur (11.6)
• "@graphql-codegen/add": "2.0.2",
• "@graphql-codegen/cli": "1.20.1",
• "@graphql-codegen/typescript": "1.21.0",
• "@graphql-codegen/typescript-operations": "1.17.14",
• "graphql": "15.5.0"

Additional context

Probably related to this PR:
#5104

[gilgardosh]

Hi @brainkim ,
could you test it with this alphas, and confirm it fixes your issue?
#6752 (comment)

[brainkim]

I tried upgrading typescript-operations to 2.1.5, but am now running into new errors related to the preResolveTypes option erasing Maybe types in an unexpected way. Looking into it.

EDIT: Opened the following issue: #6757

Thanks for the quick turnaround and hello from ApolloGraphQL💜

[gilgardosh]

Rethinking this case.
If conditional directive is used, the expected outcome must be optional, even if avoidOptinals is set.
case include is false, messages property shouldn't be in the response.
so:

export type MyQueryQuery = (

  { __typename: 'Query' }
  & { me: (
    { __typename: 'User' }
    & { messages?: Array<(
      { __typename: 'Message' }
      & Pick<Message, 'content'>
    )> }
  ) }
);

@brainkim what do you think?

[dotansimha]

I agree with @gilgardosh and @n1ru4l in this case.
skip and include will cause the field to either be there or not (the actual key in the response), so TS optional seems the correct way to represent it. avoidOptionals is a matter of choice, but in this case, it might create an inaccurate TS type...

[n1ru4l]

Just to add to that another way of representing this (without optionals could be the following):

export type MyQueryQuery = { __typename: "Query" } & {
  me: { __typename: "User" } & (
    | {}
    | { messages: Array<{ __typename: "Message" } & { content: string }> });
};

const foo = (d: MyQueryQuery) => {
  // @ts-expect-error
  d.me.messages;
  if ("messages" in d.me) {
    // do sth with it here
    d.me.messages;
  }
};

on TS Playground

This would be beneficial in a case where the boolean affects multiple fields🤔

query MyQuery($include: Boolean!) {
  message {
    id @include(if: $include)
    content @include(if: $include)
  }
}

export type MessageQuery = { __typename: "Query" } & {
  message:
    | ({ __typename: "Message" } & ({} | { id: string }) &
        ({} | { content: string }))
    | null;
};

const foo1 = (d: MessageQuery) => {
  if (!d.message) return;

  // @ts-expect-error
  d.message.id;
  // @ts-expect-error
  d.message.content;

  if ("id" in d.message) {
    // do sth with it here
    d.message.id;
  }
  if ("content" in d.message) {
    // do sth with it here
    d.message.content;
  }
};

Or with an inline fragment:

query MyQuery($include: Boolean!) {
  message {
    ... on Message @include(if: $include) {
      id
      content
    }
  }
}

export type MessageQuery = { __typename: "Query" } & {
  message:
    | ({ __typename: "Message" } & ({} | { id: string; content: string }))
    | null;
};

const foo1 = (d: MessageQuery) => {
  if (!d.message) return;

  // @ts-expect-error
  d.message.id;
  // @ts-expect-error
  d.message.content;

  if ("id" in d.message) {
    // do sth with it here
    d.message.id;
    d.message.content;
  }
};

This has the following benefits:

In the case of an optional fragment, only one property in object check has to be done in order to verify that the fragment is included.

But the following drawback:

You cannot simply do object?.optionalProp anymore without doing a "optionalProp" in object "guard" call.

In order to have the best out of both it would probably be best to settle on the following:

For single fields: Use optionalProp?: as proposed by @gilgardosh

For fragments: Use & ({} | FragmentType) as proposed by me in the inline fragment example.

[brainkim]

Ah yeah. Some disorganized thoughts.

1. The avoidOptionals option is frustrating in the sense that for input types I want optional nullable fields, and for output types I don’t. I share your intuitions that there is a “correct” generated output here, and that @skip/@include directives should be modeled with TypeScript optionals, but I think it’s confusing to have optional properties continue to appear in generated code even if you set avoidOptionals to true. I know and appreciate that graphql-code-generator tries to be supremely configurable, but I think avoidOptionals is one of those things which I wish I didn’t have control over.
2. TypeScript is undergoing a campaign to distinguish “missing” or “optional” properties from properties which can be explicitly assigned undefined (Strict optional properties microsoft/TypeScript#43947 which implements the --strictOptionalProperties option). I’m not sure how this affects codegen decisions surrounding optionals yet, but I’m sure it does somehow.
3. I dislike representing skipped/included fragments as T & ({} | TFragment) as @n1ru4l suggests because it makes it difficult to use indexed access types to access nested object or interface fields (typescript playground example). This is kind of important for Apollo because without the ability to reference subfields, migrating from apollo codegen to graphql code generator is going to be difficult. I also dislike using the in operator to duck-type, but that’s another matter.
4. Any decision we make about skipped/included fragments should likely be mirrored for modeling @defer in operations.

Thanks for the responses!

[n1ru4l]

Regarding 1: We are already considering dropping the avoidOptionals option in a future major release. For now the avoidOptionals should be adjusted to avoid optionals where it makes sense, like @gilgardosh mentioned

Regarding 3: This is something we could make configurable.

Regarding 4: skip/include and defer on fragments are IMHO best modeled with the T & ({} | TFragment) approach as they allow checking for a single property with property in obj in order to discriminate the type correctly, instead of checking for every single one. Nevertheless, we can make it configurable as part of 3

[dotansimha]

Just joining this super interesting discussion.

I think the path of TS optional is clear - it makes things more complex and could be solved with a simple explicit undefined type.
But, as @n1ru4l said, optionals are important and needed in cases where fields might really not exists at all. Mainly because of in operator that could give false positives with undefined values.

> const a = { };
> 'b' in a
false
> const a2 = { b: undefined };
> 'b' in a2
true

With skip and include, GraphQL execution layer will not return a field at all when a field was omitted due to a truthy evaluation of the directive, which means that the field is really is optional, from the point of view of the consumer.

When we started working on it, we tried to align the avoidOptionals behaviour and make it consistent as we do in other places, but now it seems like optional in this case might be important in order to reflect the actual possible responses from the GraphQL response.

@brainkim what do you think?

[brainkim]

I am agreeing with the proposal to ignore avoidOptionals for @skip/@include, and that we should probably revert #5104! I am less enthusiastic about using {} in unions. Unless there is a way to eliminate {} at the type level it will make working with operation types a little more difficult.

[brainkim]

My colleague @martinbonnin mentioned a fun little example:

type Query {
  me: User!
}

type User {
  message: Message
}

type Message {
  content: String!
}
query MyQuery($include: Boolean!) {
  me {
    message @include(if: $include) {
      content
    }
  }
}

Should message be optional and nullable or just optional or just nullable ?

Another wrinkle to think about.

[n1ru4l]

@brainkim Forthat example, the generated code should be:

export type MyQueryQuery = { __typename: "Query" } & {
  me: { __typename: "User" } & {
    messages?: Array<
      { __typename: "Message" } & Pick<Message, "content">
    > | null;
  };
};

[n1ru4l]

@brainkim Can you please confirm that with the latest version the types are generated as we expected them to be?

[brainkim]

@n1ru4l I took a brief look at the PR referenced in the CHANGELOG.md. This test seems to indicate that we’re respecting avoidOptionals and making group non-optional, even though it’s in an infline fragment which is selectively included? I’m probably just misreading the code 😅

For posterity, can you provide a brief description of what y‘all decided on with regard to avoidOptionals and inclusion directives? If there are unexpected types I can reopen this issue or create a new one.

Thank you so much for looking into this!

[gilgardosh]

@brainkim Thanks for being thorough! there actually is an issue with inline fragments mixed with conditional directive such as in this case.

The directive is not influencing user nor group directly, but the query type as a hole.
For example, result cannot be

{
  user: "userName"
}

since group is required if result exists.

The Maybe wrapping user is due to it's own definition.

Here is a fix, which you mentioned is not your cup of tea, but is a true representation of this case.

TL;DR:
<CurrentType> | {}