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

[near-operation-file preset] Include external fragments in generated output #10269

Closed
adapap opened this issue Jan 22, 2025 · 11 comments
Closed
Assignees
Labels
waiting-for-answer Waiting for answer from author

Comments

@adapap
Copy link
Contributor

adapap commented Jan 22, 2025

Is your feature request related to a problem? Please describe.

Hello! I am in the process of migrating a codebase away from Apollo Codegen (which has been deprecated for some time now) to @graphql-codegen/cli. One compatibility issue that I'm trying to address is that external fragments are not included in the output. We have GraphQL files with usages like this:

#import "./fragments.graphql"
query MyQuery {
  ...ExternalFragment
}

This does not include ExternalFragment in the generated output, but we would like to include this despite the redundancy of being able to import from the external file directly using the near-operation-file preset with typescript-operations.

Describe the solution you'd like

The relevant code is here. I'd like to have a configuration option in the typescript-operations plugin called includeExternalFragments which simply ignores this line.

Describe alternatives you've considered

While the extractAllFieldsToTypes option does generate fragments, it also generates all types for other fields which we are not at this point interested in.

Any additional important details?

No response

adapap added a commit to adapap/graphql-code-generator that referenced this issue Jan 22, 2025
This option allows you to include external fragments in the generated code.

Closes dotansimha#10269.
adapap added a commit to adapap/graphql-code-generator that referenced this issue Jan 22, 2025
This option allows you to include external fragments in the generated code.

Closes dotansimha#10269.
@eddeee888
Copy link
Collaborator

eddeee888 commented Jan 23, 2025

Hi @adapap ,
It's a bit hard for me to understand this issue without knowing your codegen config, schema, and expected outcome. Fragment support can vary depending on your setup.

Please create a minimal reproduction available on GitHub, Stackblitz or CodeSandbox.

@eddeee888 eddeee888 added the waiting-for-answer Waiting for answer from author label Jan 23, 2025
@adapap
Copy link
Contributor Author

adapap commented Jan 23, 2025

Hey @eddeee888,

Here's a Sandbox that should be a minimal repro. I'd like the generated documents/document.ts file to include the imported fragment as a type when using the near-opeartion-file preset.

It looks like in the latest versions of the typescript-operations plugin my proposed fix doesn't seem to work because the underlying visitor used is different (it's not ClientSideBaseVisitor).

@eddeee888
Copy link
Collaborator

eddeee888 commented Jan 25, 2025

Hi @adapap ,

By default, codegen doesn't import the fragment, but it includes and flattens the fragments in the generated template. From your example, I can see that given these GraphQL documents:

query user {
  user(id: 1) {
    ...ExternalUser
  }
}

fragment ExternalUser on User {
    id
    username
    email
}

The generated type is:

export type UserQuery = { 
  __typename?: 'Query', 
  user: { 
    __typename?: 'User', 
    /* Fields below come from ExternalUser */
    id: string, 
    username: string, 
    email: string 
  } 
};

So, the final generated type is correct IMO. If this is not correct, could you please let me know why and what you expect instead please?

@adapap
Copy link
Contributor Author

adapap commented Jan 25, 2025

@eddeee888 Yes, the default behavior makes sense and the codegen behavior is correct! For our specific use case, we'd like to (redundantly) have the fragment be generated as an auxillary type in the output. In the example, if the fragment is defined in the same file as the query, the codegen will output a separate type for the query and fragment itself.

We're interested in opting-in to this behavior when the fragment is imported from another file. This should be possible by processing the external fragments in the same way as fragments defined within the document.

This is technically possible with the extractAllFieldsToTypes option but we only want the extraction to happen for those fragments and not all fields in the operation (and the naming is different in this case).

I've updated the sandbox to include another document with a local fragment and you can see the difference in the generated output.

@adapap
Copy link
Contributor Author

adapap commented Feb 3, 2025

Hey @eddeee888, did you have a chance to review the above? Would like to brainstorm possible approaches here, and can explore putting up a PR depending on what approach sounds reasonable to you. I think with the above PR that I proposed, my team can write a custom plugin to extract the types for external fragments even if the near-operation-file preset doesn't use the visitor under the hood by using the ClientSideBaseVisitor directly.

@eddeee888
Copy link
Collaborator

Hi @adapap ,

Sorry, I had a small break last week. I'm trying to understand whether this would be used by others in the communities too.
Could you please help me understand your use case please? 🙂
For example, why is creating redundant types preferred over importing from generated fragment type files?

@adapap
Copy link
Contributor Author

adapap commented Feb 8, 2025

Hi @eddeee888, welcome back! The reasons for us using this are two-fold:

  • We're migrating away from Apollo Codegen and this is the behavior of that library. So, we'd have to refactor all of the callsites that rely on this behavior (a lot of callsites, not very desirable) to use the correct import
  • Especially since the behavior differs depending on whether the fragment is imported or inlined within the same file, it's less convenient for developers when composing a query with many small fragments together. I want to be able to expose the fragment type directly in the file generated near the source GraphQL file rather than having to migrate all of the application's callsites that use that imported GraphQL fragment in components.

We're favoring redundancy here to keep the scope of imports smaller in the component implementations. We already have this expectation for enums and input objects defined within a query so we don't have to write things like type A = NonNullable<NonNullable<Query["parent"]>, "a">

eddeee888 added a commit to adapap/graphql-code-generator that referenced this issue Feb 9, 2025
eddeee888 added a commit to adapap/graphql-code-generator that referenced this issue Feb 19, 2025
@eddeee888
Copy link
Collaborator

eddeee888 commented Feb 19, 2025

This is released as alpha @graphql-codegen/typescript-operations@4.5.0-rc-20250219102003-0cfdb198f6db97ac8540d9a444235fd6705a0691. Could you please try?

Thank you so much for implementing this fix! 🚀

@eddeee888 eddeee888 reopened this Feb 19, 2025
@eddeee888
Copy link
Collaborator

@adapap please help the alpha release if you have some time 🙏

@adapap
Copy link
Contributor Author

adapap commented Feb 21, 2025

Experimented with this, but unfortunately in my specific use case it didn't end up solving the problem because near-operation-file preset doesn't use ClientSideBaseVisitor, but rather TypeScriptDocumentsVisitor, which I didn't add support for in the PR (I think its a more complicated endeavour).

I was able to work around the issue by manually calling TypeScriptDocumentsVisitor with the external fragments provided by the plugin config. This works because the external fragments include the entire fragment AST so the code generator is able to produce the output for the fragments only, and I'm stitching this together with the existing generated output using a custom plugin.

Since this workaround works for me, and since it's unclear to me how to implement this configuration option with the TypeScriptDocumentVisitor, I won't pursue more changes here. If something else comes up during our migration, I'll be sure to open a new issue! Thank you for the discussion!

@eddeee888
Copy link
Collaborator

I see, thanks for teh detailed explanation. Really appreciate you working with me on this journey ! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-answer Waiting for answer from author
Projects
None yet
Development

No branches or pull requests

2 participants