Codegen fails on "AType" vs "AType!"

[davedelong]

Intended outcome:

Code generation works

Actual outcome:

Code generation does not work

How to reproduce the issue:

I'm attempting to generate some code to access the Github v4 API. I've got this query I've built using github's GraphQL API Explorer: https://gist.github.com/davedelong/f18f4aa29b1b3938e4e15dc9122ac019

In my project in Xcode 11.2, I've added the "Run Script" build phase as instructed by the documentation, and am passing in my graphql file, as well as the Github schema file I downloaded from here: https://developer.github.com/v4/public_schema/

When I attempt to build, I'm getting an error from the code generator:

./GraphQL/RecentPRs.graphql:87: error: Fields "commit" conflict because they return conflicting types Commit! and Commit. Use different aliases on the fields to fetch both if this was intentional.
./GraphQL/RecentPRs.graphql:25: error: Fields "commit" conflict because they return conflicting types Commit! and Commit. Use different aliases on the fields to fetch both if this was intentional.

Versions

I am using the latest version of Apollo.

[designatednerd]

@hwillson or @abernix any thoughts on this - here's the current SDL thing from Github:
schema.public.graphql.zip

The errors Dave is getting appear to be here:

fragment MergeFields on MergedEvent {
  id
  createdAt
  actor{...UserFields}
  commit {...CommitFields}
}

and here on the big-ass query:

... on PullRequestCommit{
  commit { ...CommitFields }
}

Is this an issue where if you've got an extension and the type is Commit, it freaks out on Commit! as the type?

[designatednerd]

OK after pulling the Github schema and trying this myself, I think I figured it out: The problem is that you have two different properties at exactly the same level with the same name. If you replace the fragments for MergeFields and CommitFields with their actual contents in the query it's a little easier to see:

query RecentPRs($owner: String!, $name: String!){
  repository(owner:$owner,name:$name) {
    [irrelevant]
    pullRequests(last:20) {
      edges {
        node {
          [irrelevant]
          timelineItems(last:20){
            edges {
              node {
                __typename
                [irrelevant]
                ... on MergedEvent {
                  id
                  createdAt
                  actor{...UserFields}
                  commit {
                    messageHeadline
                    messageBody
                    url            
                  }
                }
                [irrelevant]
                ... on PullRequestCommit{
                  commit { 
                    messageHeadline
                    messageBody
                    url 
                  }
                }
              }
            }
          }
        }
      }
    }
  }
}

Since commit is nullable on MergeEvent but not on PullRequestCommit, you've got two possible things with exactly the same name at exactly the same level, but different nullability annotations. Because our codegen doesn't know how to generate code for that, it errors out.

The easiest solution is aliasing - I'd say doing it in the merge event fragment is the best way to go:

fragment MergeFields on MergedEvent {
  id
  createdAt
  actor{...UserFields}
  mergeCommit: commit {...CommitFields}
}

I tried this and it generated code without issue.

[shayded-exe]

@designatednerd Is this something that's going to be fixed? Making queries like this is a valid thing to do. Also in TypeScript, it's possible to properly generate types for this using union types. At the very least is there some way we can hide this error?

Also, It's underlining the wrong code with the error:

[designatednerd]

The recommended solution right now is aliasing one of the fields.

It may be a perfectly valid GQL query, but unfortunately there's a difference between a) "a perfectly valid GraphQL query" and b) "a GraphQL query whose results can be represented in a type-safe fashion when accounting for nullability". The vast majority of queries are both a & b, but this is an occasion where a is true but b is not.

I'll stipulate that I'm not familiar enough with Typescript to know how a Union type could help with the same type being nullable vs. not-nullable - this issue is more related to the Swift codegen than TS codegen.

I would recommend filing a separate issue for the wrong line being underlined with the error, that's definitely a bug.

[shayded-exe]

@designatednerd Not my repo so I can't alias. Aliasing also adds complexity to consuming the data.

TypeScript can in fact represent it in a type-safe fashion. Because of that, can we please get an option to ignore this error? Even if it isn't type safe, we shouldn't be forced to have this error.

I'll file a separate issue for the underlines.

Thank you so much for the quick reply btw.

[designatednerd]

Not my repo so I can't alias.

You should be able to alias in your GQL query - the alias should then get generated as the local typename. Unless you're in a situation where you don't have control of the queries?

TypeScript can in fact represent it in a type-safe fashion.

I'm curious, how?

[shayded-exe]

Yeah the whole thing isn't my repo including the queries. This is where it's from: https://github.com/ParabolInc/parabol/blob/v5.5.0/packages/client/subscriptions/MeetingSubscription.ts

I'm planning on doing a similar thing with fragments in my own project though.

I'm curious, how?

Union types. The resulting type is the "lowest common denominator" between all the types. You have to do a type assertion to use it as one of the possible concrete types.

export type MeetingSubscriptionPayload =
  | IAddCommentSuccess
  | IAddReactjiToReflectionSuccess
  | IAddReactjiToReactableSuccess
  | IAutoGroupReflectionsPayload
  | ICreateReflectionPayload
  | IDeleteCommentSuccess
  | IDragDiscussionTopicPayload
  | IEndDraggingReflectionPayload
  | IEditReflectionPayload
  | IFlagReadyToAdvanceSuccess
  | INewMeetingCheckInPayload
  | IPromoteNewMeetingFacilitatorPayload
  | IRemoveReflectionPayload
  | ISetAppLocationSuccess
  | ISetPhaseFocusPayload
  | ISetStageTimerPayload
  | IStartDraggingReflectionPayload
  | IUpdateCommentContentSuccess
  | IUpdateDragLocationPayload
  | IUpdateNewCheckInQuestionPayload
  | IUpdateReflectionContentPayload
  | IUpdateReflectionGroupTitlePayload
  | IUpdateRetroMaxVotesSuccess
  | IVoteForReflectionGroupPayload

Simpler example:

interface A {
  __typename: 'a'
  field: number
}

interface B {
  __typename: 'b'
  field: number | null
}

type AB = A | B;

function test(obj: AB) {
  // obj.field is "number | null" here

  if (obj.__typename === 'a') {
    // obj.field is "number" here
  }
}

[designatednerd]

Right that works for things that are different types, the issue here is that one thing is Type, the optional version of the type, and the other is Type!, the non-optional version of the type. Can a union be used to represent optional and non-optional versions of the same type?

[shayded-exe]

I don't understand. These are different types we're dealing with. The union is at the top level. But yes you should be able to. This does what you're talking about.

interface Thing {
  field: Type | null;
}

Regardless of if it can be represented type-safe or not, it's still a valid option and shouldn't be forced upon the user. We aren't even using Apollo for codegen, just for the syntax highlighting and autocomplete. In the case of the repo I linked, it's using relay-compiler to do the codegen and that works just fine.

I think that goes for a lot of other users too. We just want the other features of this plugin without these codegen errors.

[designatednerd]

The error in your screenshot states the conflicting types are NewMeeting and NewMeeting! - those are optional vs. non-optional, which is why I was discussing optional vs. non-optional instead of different types. That was also the original problem that the filer of this issue was referring to.

I'll ask the tooling team if someone can clarify what's going on with the error.

[shayded-exe]

That was also the original problem that the filer of this issue was referring to.

Gotcha. Sorry, I did hijack the issue a bit.

I'll ask the tooling team if someone can clarify what's going on with the error.

Much appreciated. Please let me know what they say.

To clarify, my approach would be to have a union of the entire payload type. It doesn't matter if fields within those types clash because the response will be only one of the possible types, which of course can be represented by my A | B example.

[theogravity]

We just ran into this ourselves:

• We have subscription events that share the same interface
• Some of those events have the same field name (that is not present on the interface)
• Some of those events with the same field name may or may not be marked as nullable

Running codegen will give:

GraphQLDocumentError: Fields "fieldA" conflict because they return conflicting types "Str
ing!" and "String". Use different aliases on the fields to fetch both if this was intentional.

Our fix was to move it to the interface as optional (not ideal), and probably would have wanted the aliased approach.

export const someSubscription = gql`
  subscription someSubscription() {
      ... on EventA {
        # Is marked nullable
        fieldA
      }

      ... on EventB {
        # is not
        fieldA
      }

      ... on EventC {
        someOtherField
      }
    }
  }
`;

Is there a way to alias it in this situation?