dotansimha / graphql-code-generator

A tool for generating code based on a GraphQL schema and GraphQL operations (query/mutation/subscription), with flexible support for custom plugins.
https://the-guild.dev/graphql/codegen/
MIT License
10.79k stars 1.32k forks source link

custom scalars (config scalars) ignored since last major update #9547

Open Zerebokep opened 1 year ago

Zerebokep commented 1 year ago

Which packages are impacted by your issue?

@graphql-codegen/cli, @graphql-codegen/add, @graphql-codegen/typescript-operations, @graphql-codegen/typescript

Describe the bug

https://codesandbox.io/p/sandbox/fragrant-dream-4q295n

please check codegen.ts and the generated types.ts

p.s. kudos to the one who created that minimal reproduction template(s)

Your Example Website or App

https://codesandbox.io/p/sandbox/fragrant-dream-4q295n

Steps to Reproduce the Bug or Issue

as described

Expected behavior

as described

Screenshots or Videos

No response

Platform

Codegen Config File

No response

Additional context

No response

eddeee888 commented 1 year ago

Hi @Zerebokep 👋

I'm observing the following from the sandbox:

  1. The following custom config is created and passed to scalars option:
const scalars = {
  ID: "string",
  String: "string",
  Boolean: "boolean",
  Int: "number",
  Float: "number",
  bigint: "number",
};

Then, the generated type is this:

export type Scalars = {
  ID: { input: string; output: string; }
  String: { input: string; output: string; }
  Boolean: { input: boolean; output: boolean; }
  Int: { input: number; output: number; }
  Float: { input: number; output: number; }
};

This seems to be working correctly 🙂


Note that bigint is not added because it's not in the schema.graphql file. I'm adding the scalar bigint into schema.graphql file, the generated template is:

export type Scalars = {
  ID: { input: string; output: string; }
  String: { input: string; output: string; }
  Boolean: { input: boolean; output: boolean; }
  Int: { input: number; output: number; }
  Float: { input: number; output: number; }
  bigint: { input: number; output: number; }
};

This is also expected.


Please help me understand the issue a bit more by elaborating what you expect it to behave

Cliffzz commented 1 year ago

Hey 👋🏻 @Zerebokep thanks for looking into this. We're running into the same issue, previously the scalars type looked like this:

export type Scalars = {
  ID: string;
  String: string;
  Boolean: boolean;
  Int: number;
  Float: number;
  BigInt: any;
};

However after the update it looks like this:

export type Scalars = {
  ID: { input: string; output: string; }
  String: { input: string; output: string; }
  Boolean: { input: boolean; output: boolean; }
  Int: { input: number; output: number; }
  Float: { input: number; output: number; }
  BigInt: { input: any; output: any; }
};

I'm assuming this isn't intended as it breaks the types for the graphql query responses.

eddeee888 commented 1 year ago

Hi @Cliffzz, this change is intended 🙂

Can you help me understand how it breaks the GraphQL Query responses please? Best is to provide a reproduction using this Codesandbox template and/or some config examples

Cliffzz commented 1 year ago

@eddeee888 Thank you! Based on that I delved a little deeper and figured out our problem.

We previously defined a custom scalar in our config like this.

                scalars: {
                    Custom: 'Types.Scalars[\'Custom\'],
                },

specifying the input and output specifically resolves our issues.

                scalars: {
                    Custom: {
                        input: 'Types.Scalars[\'Custom\'][\'input\']',
                        output: 'Types.Scalars[\'Custom\'][\'output\']',
                    },
                },
Zerebokep commented 1 year ago

@eddeee888 I want to scratch the new input/output completely, basically I want the old behaviour, like this:

export type Scalars = {
  ID: string;
  String: string;
  Boolean: boolean;
  Int: number;
  Float: number;
  bigint: number;
};
eddeee888 commented 1 year ago

Hi @Zerebokep 👋 I left a comment explaining why we moved to this approach in the new major version here. I don't see us going back to maintaining both approaches

Most relevant bit related to this convo is this:

Unfortunately, maintaining both approaches in the core plugins means two things:

  • A lot of maintenance for the core maintainers
  • A lot of maintenance for the community libraries that depend on the core plugins

Adding more config options and logic means more maintenance, which hinders our ability to deliver ongoing value to the community.

Do you mind helping me understand why this new approach is causing issues for you? 🙂

Zerebokep commented 1 year ago

For the project I work for it has no use/adds unneeded complexity/overhead, and unfortunately a refactor doesn't seems that trivial, at least not at the first look. I understand for what it is used for, but it surprises me that this is something generally wanted, even if most (correct me if I'm wrong) projects don't need it, for me it sounds more like a niche feature. I always used strictly typed languages, thus I'm probably biased when it comes to variable type definitions.

Anyways, maybe we'll just stick to an old version, thanks for the clarification. :)

nashspence commented 1 year ago

Just updated and saw this as well. I also would have liked to see this implemented as not a breaking change. It seems like good new feature, but a bit brutish making this a breaking change. Anyway, thanks for the work you do maintaining this.

Zerebokep commented 1 year ago

So I'm currently checking more in detail regarding a refactor, let's say I want to mutate with "username" as parameter, originally we wrote it like that:

mutate(
      {
        username: 'test123',
      }

this converts to:

mutate(
      {
        username: { input: 'test123', output: ''},
      }

output doesn't seem to be optional, I don't think I'm doing this correctly here.

oh come one, did I really just had to install the client-preset package to get everything working again? 😅 You guys should put that in big fat red text somewhere.

eddeee888 commented 1 year ago

Hi @Zerebokep , I don't think it's an issue that needs client-preset. If you can provide your codegen setup in a Codesandbox, I can have a look. 🙂

Just looking at your example, it looks like the type provided to the mutate function is referencing Scalars['ID'] instead of Scalars['ID']['input'].

yanniznik commented 3 months ago

@Zerebokep what was your solution ? We're currently facing the same problem with custom Scalars being generated as key: { input: string; output: string; }

eddeee888 commented 3 months ago

Hi @Zerebokep , updating all codegen packages to the latest versions should work

Zerebokep commented 3 months ago

That might be the reason why it worked after installing the client-preset package, however, I usually update all packages using yarn upgrade-interactive --latest. So I'm not sure what the exact reason was why it didn't work, however, it works now.

@yanniznik might be worth a try to delete node_modules or purge the cache to re-install all packages.