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

chore(api-graphql): Bump graphql to 14.5.0 #8984

Merged
merged 13 commits into from Oct 26, 2021
Merged

chore(api-graphql): Bump graphql to 14.5.0 #8984

merged 13 commits into from Oct 26, 2021

Conversation

wlee221
Copy link
Contributor

@wlee221 wlee221 commented Oct 4, 2021

Description of changes

Bumps api-graphql to 14.5.0 to address #8983. The specific commit we need is graphql/graphql-js#2102, which adds strict types for graphql modules. This resolves TypeScript errors when you use 1) Angular 12+, and/or 2) strict mode on TypeScript.

Issue #, if available

Fixes #8983
aws-amplify/docs#2918, aws-amplify/docs#3140, aws-amplify/docs#3376

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes

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

@wlee221 wlee221 changed the title deps(api-graphql): Bump graphql to 14.5.0 chore(api-graphql): Bump graphql to 14.5.0 Oct 4, 2021
@wlee221 wlee221 marked this pull request as draft October 4, 2021 23:54
@wlee221
Copy link
Contributor Author

wlee221 commented Oct 5, 2021

Getting these errors on yarn build:

Errors ``` @aws-amplify/api-graphql: message: "/Users/willeea/Documents/amplify/amplify-js/packages/api-graphql/src/GraphQLAPI.ts (207,20): Property 'operation' does not exist on type 'DefinitionNode'.", @aws-amplify/api-graphql: level: 'error' @aws-amplify/api-graphql: } @aws-amplify/api-graphql: { @aws-amplify/api-graphql: message: "/Users/willeea/Documents/amplify/amplify-js/packages/api-graphql/src/GraphQLAPI.ts (297,17): Argument of type 'string | DocumentNode' is not assignable to parameter of type 'ASTNode'.\n" + @aws-amplify/api-graphql: " Type 'string' is not assignable to type 'ASTNode'.", @aws-amplify/api-graphql: level: 'error' @aws-amplify/api-graphql: } @aws-amplify/api-graphql: { @aws-amplify/api-graphql: message: "/Users/willeea/Documents/amplify/amplify-js/packages/api-graphql/src/GraphQLAPI.ts (392,18): Argument of type 'string | DocumentNode' is not assignable to parameter of type 'ASTNode'.\n" + @aws-amplify/api-graphql: " Type 'string' is not assignable to type 'ASTNode'.", @aws-amplify/api-graphql: level: 'error' @aws-amplify/api-graphql: } @aws-amplify/api-graphql: { @aws-amplify/api-graphql: message: "/Users/willeea/Documents/amplify/amplify-js/packages/api-graphql/src/GraphQLAPI.ts (207,20): Property 'operation' does not exist on type 'DefinitionNode'.", @aws-amplify/api-graphql: level: 'error' @aws-amplify/api-graphql: } @aws-amplify/api-graphql: { @aws-amplify/api-graphql: message: "/Users/willeea/Documents/amplify/amplify-js/packages/api-graphql/src/GraphQLAPI.ts (297,17): Argument of type 'string | DocumentNode' is not assignable to parameter of type 'ASTNode'.\n" + @aws-amplify/api-graphql: " Type 'string' is not assignable to type 'ASTNode'.", @aws-amplify/api-graphql: level: 'error' @aws-amplify/api-graphql: } @aws-amplify/api-graphql: { @aws-amplify/api-graphql: message: "/Users/willeea/Documents/amplify/amplify-js/packages/api-graphql/src/GraphQLAPI.ts (392,18): Argument of type 'string | DocumentNode' is not assignable to parameter of type 'ASTNode'.\n" + @aws-amplify/api-graphql: " Type 'string' is not assignable to type 'ASTNode'.", @aws-amplify/api-graphql: level: 'error' @aws-amplify/api-graphql: } ```

@@ -294,7 +295,7 @@ export class GraphQLAPIClass {
};

const body = {
query: print(query),
query: print(query as DocumentNode),
Copy link
Contributor Author

@wlee221 wlee221 Oct 5, 2021

Choose a reason for hiding this comment

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

This is to address "Type 'string' is not assignable to type 'ASTNode'." error.

I'm confident that this query is always of type DocumentNode, because query is always run through parsing here before _graphql function is called:

const query =
typeof paramQuery === 'string'
? parse(paramQuery)
: parse(print(paramQuery));

in which parse returns DocumentNode object (source).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either we can do as DocumentNode here, or we can create an internal type

const ParsedGraphQLOption = Omit<GraphQLOptions, 'query'> & {query: DocumentNode}

but I kept it simple for now. Opinions welcome!

Comment on lines -206 to +209
const {
definitions: [{ operation: operationType }],
} = doc;
const definitions = doc.definitions as ReadonlyArray<
OperationDefinitionNode
>;
const [{ operation: operationType }] = definitions;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not so confident with this one. It seems that doc.definitions doesn't always contain an object with operation field. You can check out its typing here: https://github.com/graphql/graphql-js/blob/7b389be745eaeda2a4f9ca33a3db93717f21447e/tstypes/language/ast.d.ts#L186-L195

OperationDefinitionNode is the one that had it, so I casted definitions to it.

Copy link
Contributor Author

@wlee221 wlee221 Oct 5, 2021

Choose a reason for hiding this comment

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

Update: I found that @types/graphql@14.0.0 has the same type, so this is not a change from 14.0.0 -> 14.5.0.

@wlee221 wlee221 marked this pull request as ready for review October 5, 2021 01:43
@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2021

Codecov Report

Merging #8984 (58bc6c4) into main (5d01a1d) will decrease coverage by 0.00%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8984      +/-   ##
==========================================
- Coverage   78.02%   78.01%   -0.01%     
==========================================
  Files         250      250              
  Lines       18109    18108       -1     
  Branches     3882     3882              
==========================================
- Hits        14129    14127       -2     
- Misses       3850     3851       +1     
  Partials      130      130              
Impacted Files Coverage Δ
packages/api-graphql/src/GraphQLAPI.ts 88.39% <33.33%> (-0.62%) ⬇️
...ages/amazon-cognito-identity-js/src/CognitoUser.js 79.37% <66.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d01a1d...58bc6c4. Read the comment docs.

Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

LGTM! Just checking if we can update to graphql@15.6.0 and import from the top of the package.

@@ -49,7 +49,7 @@
"@aws-amplify/cache": "4.0.21",
"@aws-amplify/core": "4.3.1",
"@aws-amplify/pubsub": "4.1.11",
"graphql": "14.0.0",
"graphql": "14.5.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not using graphql@15.6.0?

packages/api-graphql/src/GraphQLAPI.ts Outdated Show resolved Hide resolved
@evcodes evcodes added this to Pending Merge in Pull Requests Oct 19, 2021
@wlee221
Copy link
Contributor Author

wlee221 commented Oct 22, 2021

Update: I tried bumping it to 15.6.0, but multiple datastore e2e tests were failing -- https://app.circleci.com/pipelines/github/aws-amplify/amplify-js/9109/workflows/3489ebeb-d07a-4324-b1d8-c108b7463677.

I'm putting it back to 14.5.0 to prevent breaking changes, but we can revisit this later or when developers create an issue for this.

I'll go ahead and merge this Monday, if there are no objections. cc @elorzafe

@wlee221 wlee221 merged commit 3f8c000 into main Oct 26, 2021
Pull Requests automation moved this from Pending Merge to Merged Oct 26, 2021
@wlee221 wlee221 mentioned this pull request Dec 6, 2021
4 tasks
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2022
@jimblanc jimblanc deleted the bump-graphql-14.5.0 branch November 23, 2022 15:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

graphql Typescript build failure on strict mode
4 participants