-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
🦋 Changeset detectedLatest commit: 08ed534 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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))) |
There was a problem hiding this comment.
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.
132d828
to
5044126
Compare
@@ -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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
result[name] = { | ||
input: { | ||
isExternal: false, | ||
type: JSON.stringify(mappedScalar), | ||
}, | ||
output: { | ||
isExternal: false, | ||
type: JSON.stringify(mappedScalar), | ||
}, | ||
}; |
There was a problem hiding this comment.
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']`; |
There was a problem hiding this comment.
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.
💻 Website PreviewThe latest changes are available as preview in: https://060d15d6.graphql-code-generator.pages.dev |
3c3ef62
to
7c4196a
Compare
🚀 Snapshot Release (
|
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 ↗︎ |
93df736
to
cd298f7
Compare
- 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
ae8e835
to
5e1a7ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
Any reference to where |
@dobesv see https://spec.graphql.org/June2018/#sec-ID "Input Coercion" |
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 |
Hi @dobesv 👋 Are you generating types for clients or servers? For client types, I didn't think it would be a big change since 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'
}
},
},
},
}; |
This broke my code in angular 16. I have a query
And I was able to call it like this.
But now typescript is telling me
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? |
@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 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? |
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 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 I'm seeing two packages bringing an older version (v2.13.1) of |
Hi, in my case the new input/output structure of scalars does not work. I use the following codegen config:
In a query have a parameter (
I assigned my
|
Hi @Wytrykus , do you mind creating an issue with a sandbox please? :) |
Hi @dobesv, 👋 The default ID scalar input/output type has been reverted back to 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 🙂 |
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. |
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 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. |
Or is it that you wanted to keep using |
@dobesv , yes, the thing that was reverted was just that the input type for ID was just made to be In my opinion the faulty assumption here is that 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 |
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:
|
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 Some setups may not need this, but some setups do.
This was released as a major version. So, we expect consumers to have breaking changes.
Unfortunately, maintaining both approaches in the core plugins means two things:
Adding more config options and logic means more maintenance, which hinders our ability to deliver ongoing value to the community.
I can confirm that this assumption was not made. I refer to the generated 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
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. |
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 But in any case, it's always easy to gripe from the peanut gallery. We'll go forth using the |
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 isstring | number
.On the other hand, on the client, input type is
string | number
and ID output type isstring
.The default Scalar type will look like this:
2. Codegen config has been updated to support input/output types
A previous
typescript
config may look like this:This still works in the new implementation. This means
MyScalar
would beDate
for both input and output.Here's an example where input and output can be different:
3. All Scalar-related references need to be updated to input/output types
input
types are updated to use input Scalars4. Other breaking changes
Related #2588
Type of change
Please delete options that are not relevant.
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:
@graphql-codegen/...
:Checklist: