-
Notifications
You must be signed in to change notification settings - Fork 63
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
Conversation
c42e643
to
07fc1e2
Compare
07fc1e2
to
37460d3
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.
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", |
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.
What is this package used for? Looks like it is newly added.
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.
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.
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 |
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.
why is this extra field being added in the tests?
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.
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.
Description of changes
Upgrade
graphql
package to be inline withgraphql
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.
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
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.