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

Duplication in generated types #60

Closed
lukasluecke opened this issue Feb 23, 2020 · 21 comments · Fixed by #480
Closed

Duplication in generated types #60

lukasluecke opened this issue Feb 23, 2020 · 21 comments · Fixed by #480
Labels
enhancement New feature or request
Milestone

Comments

@lukasluecke
Copy link
Contributor

When using multiple operations it seems like all operation types are generated for each of them (the only difference between the generated files is the declare module '*/DocumentName.graphql' part). I think each file should only contain the matching types.

@piglovesyou
Copy link
Owner

You mean "the @graphql-codegen/typescript part is duplicated in all results so we should bind them up somewhere in one place", yes? If so, that's true 👍

Here I'd like to ask you (mainly to make sure the priority) what is the exact problem for that. For now, it seems not a big deal for me since:

  • It's d.ts, so it doesn't affect the production speed.
  • I don't think it effects development performance either, does it?
  • I think the graphql-codgen team doesn't have an answer for that (and I don't think they're going to handle it)

↓This is a rough image to show how the results of two GraphQL documents differ.
Screen Shot 2020-02-24 at 12 29 50

@lukasluecke
Copy link
Contributor Author

lukasluecke commented Feb 24, 2020

@piglovesyou Yes, exactly. @graphql-codegen/typescript and @graphql-codegen/typescript-resolvers should not be part of the operation types.

I've been using grapqhl-code-generator for quite some time now, and have configured it with https://graphql-code-generator.com/docs/presets/near-operation-file since that's available.

This results in separate files for the general types and the specific operation types. I'm not sure if it has any performance effects, but it's cleaner and makes sure that I import the types from the right file 🙂

image

@piglovesyou
Copy link
Owner

Thank you. Ah, what a beautiful behavior the @graphql-codegen/near-operation-file-preset does.

I didn't track the code but I'm guessing the near-operation-file-preset:

  1. knows which plugins generate duplicated code and which will not
  2. Using GraphQL AST, it flexibly generates general modules / operation-specific modules and links them by import "./common.d.ts"

The reasons it'll take long to work on this issue are:

  • The current architecture of graphql-let doesn't touch both 1. specific plugin names and 2. GraphQL AST. It can dig into it but especially the 1 will be hard to follow the latest states.
  • No benefits except "clean".

I'll take more time to see near-operation-file-preset when I have time

@lukasluecke
Copy link
Contributor Author

Yeah no worries, I see this isn't a high priority thing - just wanted to let you know 😁

The main thing is using the generates: part of the configuration to split the files / plugins, so there is no "magic" in knowing which plugins generate what, that's up to the user to configure correctly. The preset then takes baseTypesPath: types.ts as part of the presetConfig, and just imports those in everything it generates - so again, not magic.

Maybe check https://github.com/dotansimha/graphql-code-generator/blob/master/packages/presets/near-operation-file/src/resolve-document-imports.ts#L1 for how this works (possibly you could import the same helper functions to achieve similar results?)

@piglovesyou piglovesyou added the Improvement Not a bug but not an enhancement label Mar 10, 2020
@aspirisen
Copy link

Another thing if you have one file with all definitions you can use 'find usages'. Now each file has it is own definitions and you can't find everywhere in the project where i.e. Client interface is used.

@dotansimha
Copy link

dotansimha commented May 17, 2020

This should help, no? https://github.com/dotansimha/graphql-code-generator/blob/master/packages/plugins/typescript/graphql-files-modules/src/index.ts#L6-L25 @piglovesyou It allow you to set the prefix to be used, and have a more strict matching of the path. If you'll set this to the relative path of the file and the cwd, it should work and have no conflicts.

@webdeb
Copy link

webdeb commented Dec 30, 2020

I am using hasura as the graphql server and the introspection result (schema) is about 20K LOC for each *.graphql file.
It would't be a problem, if I would't want to commit the generated types to git. But I've to commit it, because I am relying on the local server, which is in the same monorepo and the actual schema is not publicly available in my pipeline.

So it would be beneficial for me, if there where a generated schema file, which could be imported by the different operation files.

I am just trying out this library, so maybe there is a solution to my problem already?

@piglovesyou
Copy link
Owner

@dotansimha Thank you for everything!! I caught up here finally. We need both presets graphql-modules and import-types, do I get it right? Still studying, but I think we can fix this issue by doing something like this internally.

schema: '**/*.graphqls'
documents: '**/*.graphql'
generates:
  ./server:
    preset: graphql-modules
    presetConfig:
      baseTypesPath: schema.ts # We always generate this
    plugins:
      - typescript
      - typescript-resolvers
  out.tsx: # This will be managed by graphql-let. All generated .tsx will import the above schema.ts.
    plugins:
      - typescript-operations
      - typescript-react-apollo
    preset: import-types
    presetConfig:
      typesPath: ./server/schema
      importTypesNamespace: SchemaTypes

@piglovesyou piglovesyou added this to the v0.18.0 milestone Mar 13, 2021
@piglovesyou piglovesyou added enhancement New feature or request and removed Improvement Not a bug but not an enhancement labels May 4, 2021
@StevenLangbroek
Copy link

Hey folks! So, we've been looking at improving our codegen set up, and are evaluating graphql-let. It looks great (thanks for this truly awesome piece of work <3 ), but this is potentially blocking us from gradual migration. Is there anything we can do to help with this? I couldn't find any WIP, has anything been started yet?

@piglovesyou
Copy link
Owner

piglovesyou commented May 9, 2021

Hi @StevenLangbroek, thanks for trying graphql-let. I had an idea and stopped, but I'm starting it again partially because of your sponsoring me😄 I can't promise, but maybe it'll publish with a couple of weeks.

Folking is also welcomed. My idea is as simple as to make the presets mentioned above be built-in. If you already know how to achieve it by GraphQL code generator's codegen.yml, you'll probably know how to change buildCodegenConfig(). Hope it helps.

@piglovesyou
Copy link
Owner

piglovesyou commented May 9, 2021

I'll leave my thought. I think graphql-let can halt to support preset and manage what they do by itself. Their preset is not a usual preset like Babel Presets but some additional generation logic affecting the plugin implementations. For example, the importOperationTypesFrom option has to be implemented by many existing plugins because of the presets, which can't happen in Babel, I guess. It seems it's difficult to stick to config-compatibility of codegen.yml and .graphql-let, and I think we don't have to.

@StevenLangbroek
Copy link

@piglovesyou oh appreciate that, but that's not why I'm sponsoring :). @acao do you think we can help with this?

@acao
Copy link

acao commented May 9, 2021

It seems what @piglovesyou is saying is that in order for graphql-let to support near-operation-file preset behavior, we will need it to support presets in the first place, or find some way to re-create the same behavior with a similar plugin using the existing plugin interface. The latter may be more practical if possible, and save graphql-let from going down the potentially precarious road of attempting more exacting config parity with graphql-config, it would seem?

@piglovesyou
Copy link
Owner

piglovesyou commented May 9, 2021

I know, @StevenLangbroek👍

@acao Thanks for investigating this and sharing your idea. Shortly, I think graphql-let can solve the problem by calling @graphql-codegen/import-types-preset internally, cofigured as (in codegen.yml format):

schema: '**/*.graphqls'
documents: '**/*.graphql'
generates:
    __generates__/__SHARED_SCHEMA__.ts:
        plugins:
            - typescript
    __generates__/a.graphql.tsx:
        plugins:
            - typescript-operations
            - typescript-react-apollo:
                importOperationTypesFrom: ''
        preset: import-types
        presetConfig:
            typesPath: ./server/schema

My previous thought the above is just a little concern/memo about config compatibility as you said; graphql-let is going to control presets.

I'm working on it on split-schema-and-document branch. Some passes, the rest still fails. I'll continue to work anyway but appreciate any of your comments. Rest of tasks:

  • tsc fails since import * as __SchemaTypes__ from '../schema/type-defs.graphqls' fails to resolve. This requires change on the current file deploy strategy. seems no problem

@piglovesyou
Copy link
Owner

Before publishing v0.18.0, I'd like you guys to double-check the problem we're solving. Do you think this is enough? Glad for reactions within 24h maximum.

Frame 11

I've published a prerelease. Please try it by yarn add -D graphql-let@latest if you have time. Thank you for you help.

@StevenLangbroek
Copy link

@acao let's give this a go 🥳

@acao
Copy link

acao commented May 18, 2021

Before publishing v0.18.0, I'd like you guys to double-check the problem we're solving. Do you think this is enough? Glad for reactions within 24h maximum.

Frame 11

I've published a prerelease. Please try it by yarn add -D graphql-let@latest if you have time. Thank you for you help.

This worked except for one bug where GraphQLErrors were being shown with trying to parse a commonjs file. I will come back to this later this week hopefully! Either way, we feel confident this will be a good alternative to near operation file for our teams!

@piglovesyou
Copy link
Owner

Thanks for taking the time for this, sounds great. Did you get that error by v0.18.0-beta.x? I've just published 0.18.0-beta.4 and I think it's about to release. I couldn't catch that kind of error in our tests, unfortunately. I'll be waiting for the clues.

@acao
Copy link

acao commented May 18, 2021

Ah my bad should’ve clarified, it was only 0.18.0-beta.1! I’ll give the latest beta release a try soon and report back! Thanks for such an awesome project!

@piglovesyou
Copy link
Owner

I published v0.18.0, fixing this issue. Thank you, guys. I'd love to hear what you think about the behavior. I wrote a migration guide for it. Please check it.

@acao
Copy link

acao commented May 19, 2021

Thank you! Will take a look soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
7 participants