-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[typescript-operations] Clean up selection set processor that depends on typescript plugin (preResolveTypes: false)
#10556
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
[typescript-operations] Clean up selection set processor that depends on typescript plugin (preResolveTypes: false)
#10556
Conversation
|
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-codegen/cli |
6.0.3-alpha-20260101134152-db5e004a3d2c64200376026e73d4a178cd47367f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/introspection |
5.0.1-alpha-20260101134152-db5e004a3d2c64200376026e73d4a178cd47367f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/visitor-plugin-common |
7.0.0-alpha-20260101134152-db5e004a3d2c64200376026e73d4a178cd47367f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typescript-document-nodes |
5.0.5-alpha-20260101134152-db5e004a3d2c64200376026e73d4a178cd47367f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/gql-tag-operations |
5.0.6-alpha-20260101134152-db5e004a3d2c64200376026e73d4a178cd47367f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typescript-operations |
6.0.0-alpha-20260101134152-db5e004a3d2c64200376026e73d4a178cd47367f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typescript-resolvers |
5.1.3-alpha-20260101134152-db5e004a3d2c64200376026e73d4a178cd47367f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typed-document-node |
6.1.3-alpha-20260101134152-db5e004a3d2c64200376026e73d4a178cd47367f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/typescript |
6.0.0-alpha-20260101134152-db5e004a3d2c64200376026e73d4a178cd47367f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/client-preset |
6.0.0-alpha-20260101134152-db5e004a3d2c64200376026e73d4a178cd47367f |
npm ↗︎ unpkg ↗︎ |
@graphql-codegen/graphql-modules-preset |
5.0.6-alpha-20260101134152-db5e004a3d2c64200376026e73d4a178cd47367f |
npm ↗︎ unpkg ↗︎ |
20e7671 to
5609723
Compare
| it('Should handle "namespacedImportName" and "preResolveTypes" together', async () => { | ||
| const testSchema = buildSchema(/* GraphQL */ ` | ||
| type Query { | ||
| f: E | ||
| user: User! | ||
| } | ||
|
|
||
| enum E { | ||
| A | ||
| B | ||
| } | ||
|
|
||
| scalar JSON | ||
|
|
||
| type User { | ||
| id: ID! | ||
| f: E | ||
| j: JSON | ||
| } | ||
| `); | ||
| const ast = parse(/* GraphQL */ ` | ||
| query test { | ||
| f | ||
| user { | ||
| id | ||
| f | ||
| j | ||
| } | ||
| } | ||
| `); | ||
| const config = { namespacedImportName: 'Types', preResolveTypes: true }; | ||
| const { content } = await plugin(testSchema, [{ location: 'test-file.ts', document: ast }], config, { | ||
| outputFile: '', | ||
| }); | ||
|
|
||
| expect(content).toMatchInlineSnapshot( | ||
| ` | ||
| "export type E = | ||
| | 'A' | ||
| | 'B'; | ||
|
|
||
| export type TestQueryVariables = Exact<{ [key: string]: never; }>; | ||
|
|
||
|
|
||
| export type TestQuery = { f: Types.E | null, user: { id: string, f: Types.E | null, j: any | null } }; | ||
| " | ||
| ` | ||
| ); | ||
|
|
||
| await validate(content, '', [`Cannot find namespace 'Types'.`]); | ||
| }); |
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 replaced by new tests such as this: https://github.com/dotansimha/graphql-code-generator/pull/10496/changes#diff-1c0111b8c7f697f38ef4de6e516943d6f07284cfd52aa19305f6a2933d8d4ba7R151-R293
| it('Should handle "namespacedImportName" and add it when specified', async () => { | ||
| const ast = parse(/* GraphQL */ ` | ||
| query notifications { | ||
| notifications { | ||
| id | ||
|
|
||
| ... on TextNotification { | ||
| text | ||
| textAlias: text | ||
| } | ||
|
|
||
| ... on TextNotification { | ||
| text | ||
| } | ||
|
|
||
| ... on ImageNotification { | ||
| imageUrl | ||
| metadata { | ||
| created: createdBy | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `); | ||
| const config = { preResolveTypes: false, namespacedImportName: 'Types' }; | ||
| const { content } = await plugin(schema, [{ location: 'test-file.ts', document: ast }], config, { | ||
| outputFile: '', | ||
| }); | ||
|
|
||
| expect(content).toMatchInlineSnapshot(` | ||
| "export type NotificationsQueryVariables = Exact<{ [key: string]: never; }>; | ||
|
|
||
|
|
||
| export type NotificationsQuery = { notifications: Array< | ||
| | ( | ||
| Pick<Types.TextNotification, 'text' | 'id'> | ||
| & { textAlias: Types.TextNotification['text'] } | ||
| ) | ||
| | ( | ||
| Pick<Types.ImageNotification, 'imageUrl' | 'id'> | ||
| & { metadata: { created: Types.ImageMetadata['createdBy'] } } | ||
| ) | ||
| > }; | ||
| " | ||
| `); | ||
| await validate(content, '', [`Cannot find namespace 'Types'.`]); | ||
| }); |
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 replaced by new tests such as this: https://github.com/dotansimha/graphql-code-generator/pull/10496/changes#diff-1c0111b8c7f697f38ef4de6e516943d6f07284cfd52aa19305f6a2933d8d4ba7R151-R293
| describe('Scalars', () => { | ||
| it('Should include scalars when doing pick', async () => { | ||
| const testSchema = buildSchema(/* GraphQL */ ` | ||
| scalar Date | ||
| type Query { | ||
| me: User | ||
| } | ||
| type User { | ||
| id: ID! | ||
| joinDate: Date! | ||
| } | ||
| `); | ||
|
|
||
| const doc = parse(/* GraphQL */ ` | ||
| query { | ||
| me { | ||
| id | ||
| joinDate | ||
| } | ||
| } | ||
| `); | ||
| const config = { preResolveTypes: false }; | ||
| const { content } = await plugin(testSchema, [{ location: 'test-file.ts', document: doc }], config, { | ||
| outputFile: '', | ||
| }); | ||
| expect(content).toContain(`Pick<User, 'id' | 'joinDate'>`); | ||
| await validate(content); | ||
| }); | ||
| }); |
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 entirely tests the preResolveTypes: false approach, so it's ok to remove.
a0436f5 to
f271167
Compare
| it('Should add __typename as optional when its not specified', async () => { | ||
| const ast = parse(/* GraphQL */ ` | ||
| query { | ||
| dummy | ||
| } | ||
| `); | ||
| const config = { preResolveTypes: false }; | ||
| const { content } = await plugin(schema, [{ location: 'test-file.ts', document: ast }], config, { | ||
| outputFile: '', | ||
| }); |
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 test is no longer the expected behaviour. So it can be removed.
| " | ||
| `); | ||
| await validate(content); | ||
| }); | ||
|
|
||
| it('Should add __typename correctly when unions are in use', async () => { |
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.
Changing this test to test correct __typename generation instead.
Previously, __typename would be optional so it'd appear in the generated template
| " | ||
| `); | ||
| await validate(content); | ||
| }); | ||
|
|
||
| it('Should add __typename correctly when interfaces are in use', async () => { | ||
| it('Should add __typename correctly when interfaces are in use and nonOptionalTypename=true', async () => { |
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.
ccdcfbe to
97ba058
Compare
97ba058 to
2a616db
Compare
typescript plugintypescript plugin (preResolveTypes: false)
Description
ts-selection-set-processor.tsis an old selection set processor that picks fromtypescriptplugin types.Since we no longer have this dependency, it should be safe to clean this up in this PR.
We can also update and remove relevant tests
Related #10496
Type of change
How Has This Been Tested?