[Bug?]: GraphQL Codegen returns undefined for nullable properties within GraphQL types

[cjreimer]

What's not working?

We are working to get our application fully typescript 'strict' mode compliant, and are using types from the Redwood type generator. On the web side, the types for nullable fields are currently indicated as optional fields. For example: the following GraphQL type:

  type OpRequestComment {
    id: Int!
    createdBy: User
    updatedBy: User
  }

resolves as the following typescript type:

export type OpRequestComment = {
  __typename?: 'OpRequestComment';
  id: Scalars['Int'];
  createdBy?: Maybe<User>;
  updatedBy?: Maybe<User>;
};

It is my understanding that the graphQL server will always return either null or the actual type, but never undefined for a nullable field. Thus, the undefined or ? part of the type appears to be incorrect. It causes us issues when we are passing props into defined interfaces that do not have undefined as part of the type.

The graphQL codegen is currently configured in the redwood codebase with:

  const options: CodegenTypes.GenerateOptions = {
    config: {}, // no extra config needed for merged schema file generation
    plugins: [{ 'schema-ast': {} }],
    pluginMap: { 'schema-ast': schemaAstPlugin },
    schema: {} as unknown as DocumentNode,
    schemaAst: loadedSchema,
    filename: redwoodProjectPaths.generated.schema,
    documents: [],
  }

The graphQL codegen utilized has an optional field called: avoidOptionals, as described on https://the-guild.dev/graphql/codegen/plugins/typescript/typescript-operations#avoidoptionals

Is there a reason why we wouldn't want to turn on avoidOptionals by default? Or perhaps would it be possible to allow us to configure this setting?

Thanks!

How do we reproduce the bug?

No response

What's your environment? (If it applies)

n/a

Are you interested in working on this?

• [ ] I'm interested in working on this

[Josh-Walker-GM]

Thanks @cjreimer! I've confirmed this is indeed the current behaviour. I'm going to loop in some people with more experience with the graphql typegen. My two cents on this is that I agree with you we should avoid the optional type if it is returned as null.

[dthyresson]

Just following up @cjreimer I'll take this issue on and going to check in with the Guild to help me understand any implications with changing this setting in codegen.

Note: I've updated RW codegen to use "client preset" when using trusted documents/persisted ops. And there is a setting for this as well (default is still false/off).

[dthyresson]

@cjreimer I was reminded by the Core Team of this https://redwoodjs.com/docs/typescript/strict-mode#manual-tweaks-to-generated-code for TS strict mode.

Would this ^^ (see more docs for use in services) address your issue?

Or would you still prefer to set that optionals in the config?

[cjreimer]

@dthyresson Thanks for your work on this!

Unfortunately, we intentionally do not use removeNulls as it does not work for us for the following reasons:

1. We rely on nulls in many cases as part of our inputs.
2. It removes all types from the input. (That could easily be fixed with some generics)

We have many properties which are nullable in the schema. For example

  areaId            Int?
  area              Area?         @relation(fields: [areaId], references: [id])

As part of update mutation inputs, we allow the input properties to be null. For example, the sdl would define

  input UpdateAssetInput {
    areaId: Int
     ...
  }

Thus, allowing the areaIdto be nullable. I know it is possible to design an interface that doesn't require nulls, but it's a reasonable amount of additional code on both the web and api side to do something like { addAreaId: Int, removeAreaId: Int }

So, to answer your question, the avoidOptionals discussed in the issue post would be helpful for typing the api side properly.

Feel free to reach out and would be glad to discuss this further

[dthyresson]

So, to answer your question, the avoidOptionals discussed in the issue post would be helpful for typing the api side properly.

Got it. Very helpful.

Let me explore some ways to set that -- and I'll try your example -- and can discuss.

Seems like "ejecting" so-to-speak or grabbing a config to set in the gen flow would be valuable regardless. So, let's see how we might do that.

[cjreimer]

Awesome, thanks @dthyresson!

[dthyresson]

@cjreimer I think the config for API code/type gen is here: https://github.com/redwoodjs/redwood/blob/a231840655906f57996bcf33ac0fb6e76657d7bc/packages/internal/src/generate/graphqlCodeGen.ts#L295

As can see, avoidOptionals is set, but just for resolvers.

I added

    avoidOptionals: {
      field: true,
      inputValue: true,
      object: true,
      defaultValue: true,
      // We do this, so that service tests can call resolvers without doing a null check
      // see https://github.com/redwoodjs/redwood/pull/6222#issuecomment-1230156868
      // Look at type or source https://shrtm.nu/2BA0 for possible config, not well documented
      resolvers: true,
    },

to test and now I see

export type OpRequestComment = {
  __typename?: 'OpRequestComment'
  comment: Scalars['String']
  id: Scalars['Int']
  user: Maybe<User>
}

where as before it was:

export type OpRequestComment = {
  __typename?: 'OpRequestComment'
  comment: Scalars['String']
  id: Scalars['Int']
  user?: Maybe<User>
}

I think I need some context from @dac09 regarding the reference Prisma resolver issue, but now I at least see where the code change would go. Just need to decide if you set some config ... or as said, merge in a custom config (which seems the most reasonable).

I'll keep digging into the codegen code to see how a custom config was to be considered (if any support exists).

[cjreimer]

Awesome, thanks @dthyresson! Sounds to me like we're on the right track!

[dthyresson]

I wonder if this might be an option:

Here customResolverFn: getResolverFnType(), checks if the tsconfig for api side is in strict mode and set the args to be optional or not ... which makes me wonder if they should always be optional in strict?

export const getResolverFnType = () => {
  const tsConfig = getTsConfigs()

  if (tsConfig.api?.compilerOptions?.strict) {
    // In strict mode, bring a world of pain to the tests
    return `(
      args: TArgs,
      obj?: { root: TParent; context: TContext; info: GraphQLResolveInfo }
    ) => TResult | Promise<TResult>`
  } else {
    return `(
      args?: TArgs,
      obj?: { root: TParent; context: TContext; info: GraphQLResolveInfo }
    ) => TResult | Promise<TResult>`
  }
}

So, what I could to is hav something that returns the setting for avoidOptionals and if in strict mode set the others an addition to resolvers ... and when not in struct mode, then just resolvers.

@cjreimer Would there be a need to set this on web type gen as well?

[dac09]

I haven't played with this in a while, but I think the main reason these are optionals are to allow you to return partials of objects when querying with prisma. If we require all of the keys - it would require the relation resolvers and services to return all of the properties, which is rarely the desired behaviour. We want to return properties only when they're requested (which is why we separate them into relation resolvers).

This problem here stems from the fact that we're tying GraphQL types to Prisma types - which is convenient to get started, but also annoying when you get to more advanced use cases.

I would avoid making this change without fully evaluating all the cases this affects - we'd need a long checklist first.