Skip to content

chore: upgrade to graphql 15 #524

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

Merged
merged 1 commit into from
Jan 24, 2023
Merged

chore: upgrade to graphql 15 #524

merged 1 commit into from
Jan 24, 2023

Conversation

dpilch
Copy link

@dpilch dpilch commented Dec 2, 2022

Description of changes

Upgrade graphql package to be inline with graphql version used in the CLI.

https://github.com/aws-amplify/amplify-cli/blob/dev/packages/amplify-cli/package.json#L91

In some rare cases both graphql 14 and 15 were resolved in the package leading to errors.

Error: Cannot use GraphQLScalarType \\"String\\" from another module or realm.

Ensure that there is only one instance of \\"graphql\\" in the node_modules
directory. If different versions of \\"graphql\\" are the dependencies of other
relied on modules, use \\"resolutions\\" to ensure only one version is installed.

https://yarnpkg.com/en/docs/selective-version-resolutions

Duplicate \\"graphql\\" modules cannot be used at the same time since different
versions may have different capabilities and behavior. The data from one
version used in the function from another could produce confusing and
spurious results.

Issue #, if available

The above error would surface when the CLI was installed via Verdaccio on the macos executor on CircleCI.

#520

Description of how you validated changes

  • Unit tests
  • E2E tests
  • Further testing is still needed

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • Breaking changes to existing customers are released behind a feature flag or major version update
  • Changes are tested using sample applications for all relevant platforms (iOS/android/flutter/Javascript) that use the feature added/modified

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dpilch dpilch requested a review from a team as a code owner December 2, 2022 22:07
@dpilch dpilch marked this pull request as draft December 2, 2022 22:07
@dpilch dpilch marked this pull request as ready for review December 5, 2022 21:59
@dpilch dpilch force-pushed the upgrade-graphql-15 branch from c42e643 to 07fc1e2 Compare January 11, 2023 19:38
@dpilch dpilch force-pushed the upgrade-graphql-15 branch from 07fc1e2 to 37460d3 Compare January 11, 2023 20:22
@dpilch dpilch changed the title chore: upgrade to graphql 15 [DO NO MERGE] chore: upgrade to graphql 15 Jan 11, 2023
@dpilch dpilch changed the title [DO NO MERGE] chore: upgrade to graphql 15 chore: upgrade to graphql 15 Jan 19, 2023
Copy link
Contributor

@AaronZyLee AaronZyLee left a comment

Choose a reason for hiding this comment

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

LGTM! One question on the release. Is it going for tagged release? If so, the base branch should be switched from main

@@ -40,12 +40,13 @@
},
"devDependencies": {
"@graphql-codegen/testing": "^1.17.7",
"graphql": "^14.5.8",
"@graphql-codegen/typescript": "^2.8.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this package used for? Looks like it is newly added.

Copy link
Author

Choose a reason for hiding this comment

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

Test would fail with the following without including this.

FAIL src/__tests__/visitors/appsync-javascript-visitor.test.ts
  ● Test suite failed to run

    Cannot find module '@graphql-codegen/typescript' from '../../node_modules/@graphql-codegen/testing/cjs/resolvers-common.js'

I think GraphQL 14 may have included this dependency before.

@dpilch
Copy link
Author

dpilch commented Jan 23, 2023

Planning for an official release. There is already a tagged release (3.3.5-graphql15.0) used for testing.

@@ -49,7 +49,8 @@ exports[`JSON output should generate JSON output for a mutation with an enum and
\\"responseName\\": \\"__typename\\",
\\"fieldName\\": \\"__typename\\",
\\"type\\": \\"String!\\",
\\"isConditional\\": false
\\"isConditional\\": false,
\\"isDeprecated\\": false
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this extra field being added in the tests?

Copy link
Author

Choose a reason for hiding this comment

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

isDepreacted became required on GraphQLField in version 15. So it now shows up on all fields including __typename. graphql/graphql-js#2469

I don't see this causing any issue though.

@AaronZyLee AaronZyLee merged commit 4307dbd into main Jan 24, 2023
@dpilch dpilch deleted the upgrade-graphql-15 branch January 30, 2023 15:19
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.

3 participants