Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[typescript-operations] Arrays nested inside named fragments are not merged together #4212

Closed
helios1138 opened this issue Jun 11, 2020 · 8 comments
Assignees
Labels
plugins waiting-for-answer Waiting for answer from author

Comments

@helios1138
Copy link

helios1138 commented Jun 11, 2020

Arrays nested inside named fragments are not merged together

for example, given the schema

type Item {
  id: ID!
  name: String!
}

type Object {
  items: [Item!]!
}

and operations

fragment Object1 on Object {
  items {
    id
  }
}

fragment Object2 on Object {
  items {
    id
    name
  }
}

fragment CombinedObject on Object {
  ...Object1
  ...Object2
}

in the generated code

import { CombinedObjectFragment } from '../../generated/documents'

const object: CombinedObjectFragment = {} as any

for (const item of object.items) console.log(item.name) // Typescript Error

you get an error

Error:(5, 51) TS2339: Property 'name' does not exist on type '{ __typename?: "Item" | undefined; id: string; }'.

Which is incorrect since both Object1 and Object2 fragments are on the same type Object and are merged together in the graphql, so the field item.name is present at runtime

I'm guessing the problem stems from the fact that combined fragment types are defined dynamically as

export type CombinedObjectFragment = (
  { __typename?: 'Object' }
  & Object1Fragment
  & Object2Fragment
);

and there are some typescripts-specific issues at play here.

I think it would be good to have an option to flatten the combined fragment types so that we still get Object1Fragment and Object2Fragment generated separately, but CombinedObjectFragment does not reference them dynamically, instead, it would be listing the fields inside explicitly.

Something similar to how preResolveTypes eliminates dynamic Pick types, but for fragments as well.

@dotansimha
Copy link
Owner

dotansimha commented Jun 15, 2020

Thanks @helios1138 , I'm trying to fix it now :)

Btw, you should be able to workaround it by using flattenSelectionSet: true

@helios1138
Copy link
Author

helios1138 commented Jun 16, 2020

I'm trying to fix it now :)

@dotansimha thank you very much!

Regarding flattenSelectionSet: can't find this option in the documentation. Did you mean flattenGeneratedTypes? If yes, I've tried that and unfortunately, it would not work for me as it avoids generating fragment types altogether and my code relies on them. Also, it does not actually work in my project (I've only tried it on simple examples) as I'm getting an error

TypeError: parentType.isTypeOf is not a function

However, I have yet find the time to create a simple reproducible example for this error :)

@dotansimha
Copy link
Owner

@helios1138 yeah you are right, it's flattenGeneratedTypes, and it does fields optimization, so it might simplify the output for your (but yeah, remove fragments...). Can you share a reproduction for that error?

@kamilkisiela tried to help me with a way to merge the fragment objects. It seems like A & B in TS doesn't merge array items correctly, so we might need some helpers for that.

@threehams
Copy link

I'm not sure about viability here, but would you accept an PR with an additional option for flattenGeneratedTypes which preserves any documents containing only fragments? They would be used only for tests and types at that point.

That would be a fix this issue, be a nice runtime improvement, and prevent the need to include fragment documents in queries.

@kohlmannj
Copy link

Would love to see a fix for this issue, as it potentially blocks us from adopting GraphQL Code Generator in one of our projects. (I will see if I can find time to look into it myself, but at least wanted to make a note of this.)

dotansimha added a commit that referenced this issue Mar 7, 2021
adjust ts validation function
remove old dependency for open
@dotansimha
Copy link
Owner

@kohlmannj @helios1138 Can you please give it a try to TypeScript 4?

I checked your given example with something like that:

fragment Object1 on Object {
  items {
    id
  }
}

fragment Object2 on Object {
  items {
    name
  }
}

I got 2 fragments generated:

   export type Object1Fragment = (
      { __typename?: 'Object' }
      & { items: Array<(
        { __typename?: 'Item' }
        & Pick<Item, 'id'>
      )> }
    );
    
    export type Object2Fragment = (
      { __typename?: 'Object' }
      & { items: Array<(
        { __typename?: 'Item' }
        & Pick<Item, 'name'>
      )> }
    );

Which eventually getting merged like that:

    export type CombinedObjectFragment = (
      { __typename?: 'Object' }
      & Object1Fragment
      & Object2Fragment
    );

I wrote a small piece of code that uses the query result and tries to access both fields:

function test (t: TestQuery) {
    for (const item of t.obj!.items) {
      console.log(item.id, item.name, item.__typename);
    }
}

And it seems fine, no errors at all with TS v4, also the auto-complete seems to work nicely:

image

Note that with TS 3, it does not work, and I get the Property 'name' does not exist on type error that @helios1138 mentioned.

I created a test in the repo (which uses the latest TS 4) and the test passes compilation with no errors: #5665

Can you please confirm the above and let me know if it works correctly?

@dotansimha
Copy link
Owner

I'm trying to fix it now :)

@dotansimha thank you very much!

Regarding flattenSelectionSet: can't find this option in the documentation. Did you mean flattenGeneratedTypes? If yes, I've tried that and unfortunately, it would not work for me as it avoids generating fragment types altogether and my code relies on them. Also, it does not actually work in my project (I've only tried it on simple examples) as I'm getting an error

TypeError: parentType.isTypeOf is not a function

However, I have yet find the time to create a simple reproducible example for this error :)

The TypeError: parentType.isTypeOf is not a function was fixed a long time ago. so if you are dealing with that, just update the latest :)

@dotansimha dotansimha added the waiting-for-answer Waiting for answer from author label Mar 7, 2021
dotansimha added a commit that referenced this issue Mar 7, 2021
adjust ts validation function
remove old dependency for open
dotansimha added a commit that referenced this issue Mar 7, 2021
@ardatan
Copy link
Collaborator

ardatan commented Apr 16, 2021

Closed due to the lack of response. Feel free to create a new issue if the problem persists.

@ardatan ardatan closed this as completed Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugins waiting-for-answer Waiting for answer from author
Projects
None yet
Development

No branches or pull requests

5 participants