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

Update typescript, typescript-operations and typescript-resolvers plugins Scalars input/output type #9375

Merged
merged 6 commits into from
May 15, 2023

Conversation

eddeee888
Copy link
Collaborator

@eddeee888 eddeee888 commented May 7, 2023

Description

1. Extend Scalars base type to have input/output types.

This allows us to be able to better type the Scalar types as input type and output type could be different.

For example, on the server, ID input type is string and ID output type is string | number.
On the other hand, on the client, input type is string | number and ID output type is string.

The default Scalar type will look like this:

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

2. Codegen config has been updated to support input/output types

A previous typescript config may look like this:

config: {
  scalars: {
    MyScalar: 'Date'
  }
}

This still works in the new implementation. This means MyScalar would be Date for both input and output.

Here's an example where input and output can be different:

config: {
  scalars: {
    MyScalar: {
      input: 'Date',
      output: 'string'
    }
  }
}

3. All Scalar-related references need to be updated to input/output types

  • GraphQL input types are updated to use input Scalars
  • GraphQL object types (a.k.a output types) are updated to use output Scalar
  • Document variables are updated to use input Scalars
  • Selection sets are updated to use output Scalars
  • Resolver types are updated to use output Scalars

4. Other breaking changes

  • If a file path is used as Scalars, all scalars exported in this module must be updated to support input/output types.
config: {
  scalars: '/path/to/scalars'
}

Related #2588

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test Environment:

  • OS: MacOS
  • @graphql-codegen/...:
  • NodeJS: 18

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@changeset-bot
Copy link

changeset-bot bot commented May 7, 2023

🦋 Changeset detected

Latest commit: 08ed534

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@graphql-codegen/visitor-plugin-common Minor
@graphql-codegen/typescript-operations Minor
@graphql-codegen/typescript Minor
@graphql-codegen/typescript-resolvers Minor
@graphql-codegen/typescript-document-nodes Patch
@graphql-codegen/gql-tag-operations Patch
@graphql-codegen/typed-document-node Patch
@graphql-codegen/graphql-modules-preset Patch
@graphql-codegen/client-preset Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -677,7 +689,7 @@ export class BaseTypesVisitor<
if (this.config.onlyOperationTypes || this.config.onlyEnums) return '';
const originalNode = parent[key] as UnionTypeDefinitionNode;
const possibleTypes = originalNode.types
.map(t => (this.scalars[t.name.value] ? this._getScalar(t.name.value) : this.convertName(t)))
.map(t => (this.scalars[t.name.value] ? this._getScalar(t.name.value, 'output') : this.convertName(t)))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scalars in union types are always output.

@eddeee888 eddeee888 force-pushed the update-typescript-input-output-type branch from 132d828 to 5044126 Compare May 7, 2023 13:10
@@ -56,7 +56,7 @@ export class PreResolveTypesProcessor extends BaseSelectionSetProcessor<Selectio
(this.config.namespacedImportName ? `${this.config.namespacedImportName}.` : '') +
this.config.convertName(baseType.name, { useTypesPrefix: this.config.enumPrefix });
} else if (this.config.scalars[baseType.name]) {
typeToUse = this.config.scalars[baseType.name];
typeToUse = this.config.scalars[baseType.name].output;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used in selection set so they are always output.

@@ -87,7 +87,7 @@ export class PreResolveTypesProcessor extends BaseSelectionSetProcessor<Selectio
}
const fieldObj = schemaType.getFields()[aliasedField.fieldName];
const baseType = getBaseType(fieldObj.type);
let typeToUse = this.config.scalars[baseType.name] || baseType.name;
let typeToUse = this.config.scalars[baseType.name]?.output || baseType.name;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used in selection set so they are always output.

Comment on lines +363 to +372
result[name] = {
input: {
isExternal: false,
type: JSON.stringify(mappedScalar),
},
output: {
isExternal: false,
type: JSON.stringify(mappedScalar),
},
};
Copy link
Collaborator Author

@eddeee888 eddeee888 May 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, this was stringifying any object:

result[name] = {
  isExternal: false,
  type: JSON.stringify(scalarsMapping[name]),
};

I cannot find tests related to this case.

However, this behaviour is preserved if the mapper object does not have input/output fields.
If there's input/output fields in the mapper, we'll just use the values without stringifying them

@@ -60,7 +60,7 @@ export class OperationVariablesToObject {
protected getScalar(name: string): string {
const prefix = this._namespacedImportName ? `${this._namespacedImportName}.` : '';

return `${prefix}Scalars['${name}']`;
return `${prefix}Scalars['${name}']['input']`;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scalar types used in variables are always input.

@eddeee888 eddeee888 changed the title Update typescript and document plugins Scalars input/output type Update typescript, typescript-operations and typescript-resolvers plugins Scalars input/output type May 8, 2023
@eddeee888 eddeee888 changed the base branch from base-scalar-input-output to master May 8, 2023 11:53
@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023

💻 Website Preview

The latest changes are available as preview in: https://060d15d6.graphql-code-generator.pages.dev

@eddeee888 eddeee888 force-pushed the update-typescript-input-output-type branch from 3c3ef62 to 7c4196a Compare May 8, 2023 12:44
@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@graphql-codegen/cli 3.3.2-alpha-20230514055700-08ed5348c npm ↗︎ unpkg ↗︎
@graphql-codegen/visitor-plugin-common 3.2.0-alpha-20230514055700-08ed5348c npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript-document-nodes 3.0.5-alpha-20230514055700-08ed5348c npm ↗︎ unpkg ↗︎
@graphql-codegen/gql-tag-operations 3.0.2-alpha-20230514055700-08ed5348c npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript-operations 3.1.0-alpha-20230514055700-08ed5348c npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript-resolvers 3.3.0-alpha-20230514055700-08ed5348c npm ↗︎ unpkg ↗︎
@graphql-codegen/typed-document-node 4.1.0-alpha-20230514055700-08ed5348c npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript 3.1.0-alpha-20230514055700-08ed5348c npm ↗︎ unpkg ↗︎
@graphql-codegen/client-preset 3.1.0-alpha-20230514055700-08ed5348c npm ↗︎ unpkg ↗︎
@graphql-codegen/graphql-modules-preset 3.1.4-alpha-20230514055700-08ed5348c npm ↗︎ unpkg ↗︎
@graphql-codegen/plugin-helpers 4.2.1-alpha-20230514055700-08ed5348c npm ↗︎ unpkg ↗︎

@eddeee888 eddeee888 force-pushed the update-typescript-input-output-type branch 2 times, most recently from 93df736 to cd298f7 Compare May 9, 2023 13:21
eddeee888 and others added 5 commits May 13, 2023 16:38
- Use Scalars input/output in base typescript plugin
- Use Scalars in typescript-operations plugin
- Update typescript-resolvers to support input/output
- Update related website docs
@eddeee888 eddeee888 force-pushed the update-typescript-input-output-type branch from ae8e835 to 5e1a7ae Compare May 13, 2023 06:42
Copy link
Collaborator

@saihaj saihaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@dobesv
Copy link
Contributor

dobesv commented Jun 5, 2023

Any reference to where ID could be a number? The docs seem to say it is always a string.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jun 5, 2023

@dobesv see https://spec.graphql.org/June2018/#sec-ID "Input Coercion"

@dobesv
Copy link
Contributor

dobesv commented Jun 6, 2023

Hmm that doesn't specifically mandate support for number, and also mentions GUID and other things. I wonder if the change here would have been less disruptive if it stuck with string as the default for backwards compatibility and offered people the chance to add alternative types if they are using them.

@eddeee888
Copy link
Collaborator Author

Hi @dobesv 👋 Are you generating types for clients or servers?

For client types, I didn't think it would be a big change since number can now be sent as well as string.
For server types, ID input is always coerced into string when it gets to the resolvers, and resolvers can return string | number so the following config is required:

const config: CodegenConfig = {
   // ...
   generates: {
     'path/to/file': {
       // plugins...
       config: {
         scalars: {
           ID: { 
             input: 'string', 
             output: 'string | number' 
           },
         }
       },
     },
   },
};

Alternatively, if you want the previous behaviour, this can be used:

const config: CodegenConfig = {
   // ...
   generates: {
     'path/to/file': {
       // plugins...
       config: {
         scalars: {
           ID: 'string'
         }
       },
     },
   },
};

@burner
Copy link

burner commented Jun 8, 2023

This broke my code in angular 16.

I have a query

query GetDataAt($personId: Int!, $time: DateTime!) {

And I was able to call it like this.

const vars = { personId: theId, time: theTime };
		this.theData = new GetDataAtGQL(this.apollo)
			.watch(vars)

But now typescript is telling me

Argument of type '{ personId: { input: number; }; time: Date; }' is not assignable to parameter of type 'Exact<{ personId: { input: number; output: number; }; time: { input: any; output: any; }; }>'.
  Types of property 'personId' are incompatible.
    Property 'output' is missing in type '{ input: number; }' but required in type '{ input: number; output: number; }'.

Which technically is correct, but is quite annoying as I'm using the variables as input and not as output.

Am I doing something wrong here?

@mikeverf
Copy link

mikeverf commented Jun 13, 2023

@eddeee888 https://codesandbox.io/p/sandbox/priceless-swanson-vccd6p

I created a codesandbox where you can see that a types file is generated with the new scalar types (with input & output)

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

You'll find there's no way to override those scalar types to become the old primitive-based ones.

In api.hooks.ts you'll find it's importing from that general types file, which will resolve id to be of type { input: string | number, output: string }.

export type GetItemQueryVariables = Types.Exact<{
  id: Types.Scalars['ID'];
}>;

To me it seems it should actually be

export type GetItemQueryVariables = Types.Exact<{
  id: Types.Scalars['ID']['input'];
}>;

but can't find a way to get that to work. Could this be a bug? Do I file an issue?

@eddeee888
Copy link
Collaborator Author

Hi @mikeverf, thanks for providing the sandbox! It makes it much easier to understand the setup 🙌

In your case, you'd have to also bump @graphql-codegen/typescript-operations to v4.0.0 like this example

Here's the updated generated template with input reference:

export type GetItemQueryVariables = Types.Exact<{
  id: Types.Scalars['ID']['input'];
}>;

Please note that the core Scalar setup lives in @graphql-codegen/visitor-plugin-common v4.

I'm seeing two packages bringing an older version (v2.13.1) of @graphql-codegen/visitor-plugin-common in the yarn.lock: @graphql-codegen/near-operation-file-preset and @graphql-codegen/typescript-react-query.
In this case it's still resolving the correct package. However, it's best to bump those as well to ensure there's only one version of the package. They should live in the community repo:

@Wytrykus
Copy link

Wytrykus commented Jun 15, 2023

Hi, in my case the new input/output structure of scalars does not work. I use the following codegen config:

     plugins:
      - 'typescript'
      - 'typescript-operations'
      - 'typescript-graphql-request'
    config:
      avoidOptionals: true
      skipTypename: true
      typesPrefix: 'Foundation'

In a query have a parameter (tenantId) which is GraphQL type String and in the generated code I have

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

I assigned my tenantId variable first only to input, then also to both input and output field of the parameter object in the generated code for my query, but I always got an error response from the GraphQL server:

ClientError: Graphql validation error

@eddeee888
Copy link
Collaborator Author

Hi @Wytrykus , do you mind creating an issue with a sandbox please? :)

@eddeee888
Copy link
Collaborator Author

Hi @dobesv, 👋

The default ID scalar input/output type has been reverted back to string in the latest release: #9501

Note that the generated input/output will still be the format going forward. If there's any issues related to the Scalar input/output types being referenced incorrectly, please create a separate issue and tag me. Happy to look at them 🙂

@dobesv
Copy link
Contributor

dobesv commented Jun 20, 2023

FWIW the workaround here was easy enough, setting it explicitly to string for both, but I can imagine that making 99% of people using the tool make that change is probably unnecessary busywork since I assume the vast majority are in fact using strings.

I'm glad to see it changed to be backwards compatible again even though I already put in the fix in our codebase.

@adevine
Copy link

adevine commented Jun 21, 2023

Just a comment about this - this was a major breaking change for us, and it would be really helpful if the new Scalars behavior was more easily customizable to generate the old way across the board (i.e. just primitives, no input/output).

We use things like someProp: Scalars['ID']; all over the place in our code because it essentially provides better documentation. We have a lot of custom scalars for which we do additional validation (e.g. email addresses) and doing something like emailAddress: Scalars['EmailString'] really helps inform the developer that the property should conform to our EmailString rules, even if under the covers it just resolves to string.

Is there any way to revert this to get the old behavior across the board?

@dobesv
Copy link
Contributor

dobesv commented Jun 21, 2023

Is there any way to revert this to get the old behavior across the board?

Was it not reverted in #9501 ?

If that wasn't released you may want to just stay on an older version until it is released.

@dobesv
Copy link
Contributor

dobesv commented Jun 21, 2023

Or is it that you wanted to keep using Scalars["ID"] but now you would have to use Scalars["ID"]["input"] ? Hmm, interesting.

@adevine
Copy link

adevine commented Jun 21, 2023

@dobesv , yes, the thing that was reverted was just that the input type for ID was just made to be string instead of string | number, but the input/output object structure was still kept.

In my opinion the faulty assumption here is that Scalars would only be used internally for properties that codegen also creates, but we use it extensively as-is throughout our entire codebase for better documentation. Also, in reality, I think ID is a bit of special case in that it basically just supports the number coercion to string as a sort of "helpful edge case", but now even that has just been reverted back to string, so I really question how much of a need it is to have separate input/output types for scalars.

Edit: One other thought about this, if you still wanted to support definitions for separate input and output Scalars (again, I question the utility, but I can see how some might want that) I think it would have been much less destructive to instead of changing the Scalars structure entirely to instead add a separate InputScalars class that, by default, just defined all the properties in terms of Scalars (e.g. type InputScalars = { ID: Scalars['ID']; String: Scalars['String']; ... }). 99% of the time the input and output types would be the same, so you would only need to worry about essentially "overriding" the InputScalars type in rare cases. This would have also had the benefit of being fully backwards compatible.

@adevine
Copy link

adevine commented Jun 22, 2023

Just adding this note here in case anyone is in the same predicament we are. While for now we've reverted our code to a pre 4.x version, the following type definition basically gives you the Scalars definition in the "old" format, so we'll use this as a way to decouple from the changes made in this PR:

type FixedScalars = { [Property in keyof Scalars]: Scalars[Property]['output'] };

@eddeee888
Copy link
Collaborator Author

Hello @adevine,

Short answer: scalar input/output type will not be reverted.

Long answer:

Scalar can always be used as either input or output in GraphQL. So, having only one type for both input/output does not allow types to be set correctly in many scenarios. For example, resolvers could never be typed to take string as ID input and return string | number for IDs until this change.

Some setups may not need this, but some setups do.

Just a comment about this - this was a major breaking change for us

This was released as a major version. So, we expect consumers to have breaking changes.

, and it would be really helpful if the new Scalars behavior was more easily customizable to generate the old way across the board (i.e. just primitives, no input/output).

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

  1. A lot of maintenance for the core maintainers
  2. 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.

In my opinion the faulty assumption here is that Scalars would only be used internally for properties that codegen also creates, but we use it extensively as-is throughout our entire codebase for better documentation.

I can confirm that this assumption was not made. I refer to the generated Scalars in professional and production settings as well (maybe in different use cases than what you may have). So, I’m aware that this is a breaking change.

Personally, I fixed my input/output references based on the scalar flow. But if you have the same input and output type, one option is to find all references to Scalars['EmailString'] and change to Scalars['EmailString']['output'] (or using your proposed approach)?

Also, in reality, I think ID is a bit of special case in that it basically just supports the number coercion to string as a sort of “helpful edge case”, but now even that has just been reverted back to string, so I really question how much of a need it is to have separate input/output types for scalars.

There’s the use case of custom scalars. In general, custom scalars can take any number of incoming types and coerced the value into one type (regardless of whether the scalar is used as input or output).

Ultimately, I believe this is a necessary evil. We needed to make this breaking change to make types more flexible and correct.

@adevine
Copy link

adevine commented Jun 26, 2023

Thanks for the response @eddeee888 . And my apologies, I didn't mean to come across as overly harsh.

I fully understand the custom scalars issue (we use custom scalars heavily), I just think it would have been less detrimental to add a second InputScalars type while leaving the existing Scalars type as-is. That would have made it possible to support this requirement in a completely backwards-compatible fashion, and it also means you only have to worry about InputScalars if you actually need to have separate types for input and output (which is much less common than the default when they are the same), as the default would just be to have InputScalars essentially inherit from Scalars.

But in any case, it's always easy to gripe from the peanut gallery. We'll go forth using the { [Property in keyof Scalars]: Scalars[Property]['output'] } approach to make our migration less painful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants