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

Fix scalar input/output object bug when given external file or module pattern #9418

Merged
merged 10 commits into from
May 23, 2023

Conversation

eddeee888
Copy link
Collaborator

@eddeee888 eddeee888 commented May 16, 2023

Description

1. Fix a missed case when handling input/output scalar object

This fixes the issue originally implemented in #9375

Previously, we print the input/output value without parsing it through mapper. This means with the following config:

scalars: {
  MyScalar: {
    input: '@org/scalars#Input',
    output: '@org/scalars#Output'
  }
}

It will print the following invalid TypeScript syntax 😓 :

export type Scalars = {
  MyScalar:  {
    input: @org/scalars#Input
    output: @org/scalars#Output
  }
}

Now, input/output scalar object should now handle file and module import mapper consistently i.e. import at the top and reference the imports in the Scalars object:

import { Input } from '@org/scalars';
import { Output } from '@org/scalars';

export type Scalars = {
  MyScalar:  {
    input: Input
    output: Output
  }
}

2. Ensure backward compatibility when using mapped Scalars

Previous implementation expects external mapped scalar cases have input/output object. Below is the config for these cases

scalars: {
  MyScalar: '@org/scalars#MyScalar',
  MyRelativeScalar: './scalars#MyRelativeScalar',
}

Previously the following would be generated

import { MyScalar } from '@org/scalars';
import { MyRelativeScalar } from './scalars';

export type Scalars = {
  MyScalar:  {
    input: MyScalar['input']
    output: MyScalar['output']
  }
  MyRelativeScalar: {
    input: MyScalar['input']
    output: MyScalar['output']  
  }
}

This is big breaking change for downstream users as all mapped scalar currently is a type instead of an input/output object. This PR updates it so mapped scalars will be used for both input and output:

import { MyScalar } from '@org/scalars';
import { MyRelativeScalar } from './scalars';

export type Scalars = {
  MyScalar:  {
    input: MyScalar
    output: MyScalar
  }
  MyRelativeScalar: {
    input: MyScalar
    output: MyScalar
  }
}

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

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

  • Unit test

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 16, 2023

⚠️ No Changeset found

Latest commit: 904521c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@eddeee888 eddeee888 changed the title Fix scalar input output with external Fix scalar input/output object bug when given external file or module pattern May 16, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 16, 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-20230522233947-904521c12 npm ↗︎ unpkg ↗︎
@graphql-codegen/introspection 3.0.2-alpha-20230522233947-904521c12 npm ↗︎ unpkg ↗︎
@graphql-codegen/visitor-plugin-common 4.0.0-alpha-20230522233947-904521c12 npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript-document-nodes 3.0.5-alpha-20230522233947-904521c12 npm ↗︎ unpkg ↗︎
@graphql-codegen/gql-tag-operations 3.0.2-alpha-20230522233947-904521c12 npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript-operations 4.0.0-alpha-20230522233947-904521c12 npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript-resolvers 4.0.0-alpha-20230522233947-904521c12 npm ↗︎ unpkg ↗︎
@graphql-codegen/typed-document-node 4.1.0-alpha-20230522233947-904521c12 npm ↗︎ unpkg ↗︎
@graphql-codegen/typescript 4.0.0-alpha-20230522233947-904521c12 npm ↗︎ unpkg ↗︎
@graphql-codegen/client-preset 3.1.0-alpha-20230522233947-904521c12 npm ↗︎ unpkg ↗︎
@graphql-codegen/graphql-modules-preset 3.1.4-alpha-20230522233947-904521c12 npm ↗︎ unpkg ↗︎
@graphql-codegen/testing 2.0.3-alpha-20230522233947-904521c12 npm ↗︎ unpkg ↗︎
@graphql-codegen/plugin-helpers 4.2.1-alpha-20230522233947-904521c12 npm ↗︎ unpkg ↗︎

@@ -1997,6 +2009,9 @@ describe('TypeScript', () => {
MyScalar: { input: MyScalar['input']; output: MyScalar['output']; }
MyOtherScalar: { input: MyOtherScalar['input']; output: MyOtherScalar['output']; }
MyAliasedScalar: { input: AliasedScalar['input']; output: AliasedScalar['output']; }
OrgScalar: { input: OrgScalar['input']; output: OrgScalar['output']; }
OrgOtherScalar: { input: OrgOtherScalar['input']; output: OrgOtherScalar['output']; }
OrgAliasedScalar: { input: OrgAliasedScalar['input']; output: OrgAliasedScalar['output']; }
Copy link
Collaborator Author

@eddeee888 eddeee888 May 16, 2023

Choose a reason for hiding this comment

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

I think it would have much better backward compatibility if we don't require mapped scalar from external sources to be an input/output object

Will push a commit to fix this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 2665abf

Copy link
Contributor

Choose a reason for hiding this comment

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

You've already done it, but agreed, this last commit is quite nice. Means the only code change we had to do in our (internal) project was stop using Scalars[''] or add the ['input'] / ['output'] type.

@github-actions
Copy link
Contributor

github-actions bot commented May 19, 2023

💻 Website Preview

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

@n1ru4l n1ru4l merged commit 9e8bd9d into master May 23, 2023
@n1ru4l n1ru4l deleted the fix-scalar-input-output-with-external branch May 23, 2023 05:56
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.

4 participants