Graphql 17 support #185

igrlk · 2022-11-14T20:41:30Z

👋 Hi. I didn't get a response for this question about graphql@17 support and decided to go ahead and make graphql-jit work with the latest graphql-js

This PR stops using things that were deprecated in graphql@17 and doesn't add any new functionality like @defer support - thankfully graphql spec says that it's not required to support @stream/@defer directives to be fully spec compatible.

I think we don't need to merge it into main branch until graphql@17 is released

Happy to hear any feedback from the contributors on this. Cheers! 🎩

oporkka · 2023-01-26T11:25:33Z

Thanks for your contribution @igrlk. We added already test runs for the alpha version of 17. It seems that the tests that run also with GraphQL 15 and GraphQL 16 need fixing. Do you want to look into it to add support for older versions? We have version-specific code in style

graphql-jit/src/__tests__/json.test.ts

Lines 133 to 139 in 3cbb8b1

    
           const context: any = 
        
             versionInfo.major > 15 
        
               ? (buildExecutionContext as any)({ 
        
                   schema: blogSchema, 
        
                   document 
        
                 }) 
        
               : (buildExecutionContext as any)(blogSchema, document);

igrlk · 2023-01-27T19:15:13Z

@oporkka thanks for checking this PR! Yep I'm happy to fix the tests for other versions - would you mind allowing me to run the github checks? It will make the process faster :)

…Type syntax since old versions were deprecated and removed from graphql@17

igrlk · 2023-01-27T19:59:56Z

Hey @oporkka I was able to get the tests green on node 14, 16, 18 in combo with graphql 15, 16, 17 locally. Pushed all the changes to the branch

oporkka · 2023-01-27T22:04:00Z

Tests allowed, but fail on coverage. I am not sure why they start failing after the change, is it actually the support for version info that causes the branch coverage to degrade? In any case, propose to lower the target for now to 91% to make all the tests run

igrlk · 2023-01-27T22:17:37Z

Decreased the coverage threshold for branches to 91.

Still can't run the tests btw, looks like I need to become a contributor in the repo first to be able to run them

igrlk · 2023-01-29T18:51:05Z

@oporkka thanks for running the tests. Since I've added the additional versionInfo.major checks and the package still works with the old versions of graphql, do you think it's something we can merge now? Would love to hear your thoughts on that :)

src/ast.ts

boopathi · 2023-01-30T10:06:19Z

src/format-error.ts

+import * as utilities from "graphql/error";
+import { GraphQLFormattedError } from "graphql/error";
+
+export const formatError = (error: GraphQLError): GraphQLFormattedError => {


nit: please refactor to function declaration instead of using arrow function expression.

boopathi · 2023-01-30T10:07:47Z

src/get-graphql-error-options.ts

+import { ASTNode } from "graphql/language/ast";
+import { GraphQLError, versionInfo } from "graphql";
+
+export const getGraphQLErrorOptions = (


move this to the src/compat.ts file

boopathi · 2023-01-30T10:08:12Z

src/compat.ts

+export const getOperationRootType = (
+  schema: GraphQLSchema,
+  operation: OperationDefinitionNode
+): GraphQLObjectType => {


nit: please refactor this arrow function expression to function declaration.

oporkka · 2023-01-31T11:27:17Z

package.json

        "functions": 96,
        "lines": 96,
        "statements": 96
      }
    }
  },
  "peerDependencies": {
-    "graphql": ">=15"
+    "graphql": ">=17"


This we should keep as >=15 for now, as we still support 15

oporkka · 2023-01-31T11:29:09Z

package.json

@@ -72,7 +72,7 @@
    "eslint-plugin-node": "^11.1.0",
    "eslint-plugin-promise": "^5.1.1",
    "eslint-plugin-standard": "^5.0.0",
-    "graphql": "^16.0.0",
+    "graphql": "^17.0.0-alpha.2",


This could be also changed back to 16 before merging, as 17 is nevertheless still alpha. We will keep support in test runs, though.

src/ast.ts

igrlk · 2023-01-31T17:24:40Z

Thank you for your comments, @boopathi @oporkka. All the things you mentioned should be fixed now

igrlk · 2023-02-01T15:31:16Z

Fixed the tests

oporkka · 2023-02-02T08:59:06Z

.idea/modules.xml

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>


Let's remove this file. I propose to have .idea/ in your global .gitignore file.

Thanks for removing the file from the PR and for the global .gitignore suggestion :)

oporkka · 2023-02-02T11:04:02Z

👍

igrlk · 2023-03-10T13:24:23Z

Hi @boopathi @oporkka. Is there something that's blocking us from merging this PR? 😄

boopathi · 2023-03-13T10:14:39Z

👍

oporkka · 2023-03-13T10:24:24Z

Is there something that's blocking us from merging this PR? 😄

@igrlk Sorry for the delay, we just forgot to follow up on this as other things popped up.

igrlk mentioned this pull request Nov 14, 2022

feat: @defer support mercurius-js/mercurius#898

Open

1 task

igrlk and others added 7 commits January 27, 2023 19:44

bumped graphql dependency to 17, migrated to new GraphQLError/getRoot…

63f8b64

…Type syntax since old versions were deprecated and removed from graphql@17

bump dependencies

2a0407c

Test with GraphQL 17

83ae6f5

Specify alpha separately

07dfe5e

graphql-17: support graphql version 16

9cff6e1

graphql-17: support graphql version 15

887937c

graphql-17: updated yarn.lock

f27a194

igrlk force-pushed the graphql-17-support branch from 41f459c to f27a194 Compare January 27, 2023 19:45

graphql-17: returned back to compat.ts

aae9d94

igrlk force-pushed the graphql-17-support branch from 468f612 to aae9d94 Compare January 27, 2023 19:53

graphql-17: fix some TS errors

c68180a

graphql-17: decrease coverage threshold for branches from 92 to 91

4133630

boopathi reviewed Jan 30, 2023

View reviewed changes

oporkka reviewed Jan 31, 2023

View reviewed changes

graphql-17: code review feedback

436f6d5

igrlk commented Jan 31, 2023

View reviewed changes

src/ast.ts Show resolved Hide resolved

graphql-17: fix failing tests

ffb1ae0

oporkka reviewed Feb 2, 2023

View reviewed changes

Remove idea modules file

f3a70c1

boopathi approved these changes Mar 13, 2023

View reviewed changes

boopathi merged commit 3d2675a into zalando-incubator:main Mar 13, 2023

boopathi mentioned this pull request Mar 13, 2023

GraphQL 17 support? #184

Closed

igrlk deleted the graphql-17-support branch March 13, 2023 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graphql 17 support #185

Graphql 17 support #185

igrlk commented Nov 14, 2022

oporkka commented Jan 26, 2023

igrlk commented Jan 27, 2023

igrlk commented Jan 27, 2023

oporkka commented Jan 27, 2023

igrlk commented Jan 27, 2023

igrlk commented Jan 29, 2023 •

edited

Loading

boopathi Jan 30, 2023

boopathi Jan 30, 2023

boopathi Jan 30, 2023

oporkka Jan 31, 2023

oporkka Jan 31, 2023

igrlk commented Jan 31, 2023

igrlk commented Feb 1, 2023

oporkka Feb 2, 2023

igrlk Feb 2, 2023

oporkka commented Feb 2, 2023

igrlk commented Mar 10, 2023

boopathi commented Mar 13, 2023

oporkka commented Mar 13, 2023 •

edited

Loading

Graphql 17 support #185

Graphql 17 support #185

Conversation

igrlk commented Nov 14, 2022

oporkka commented Jan 26, 2023

igrlk commented Jan 27, 2023

igrlk commented Jan 27, 2023

oporkka commented Jan 27, 2023

igrlk commented Jan 27, 2023

igrlk commented Jan 29, 2023 • edited Loading

boopathi Jan 30, 2023

Choose a reason for hiding this comment

boopathi Jan 30, 2023

Choose a reason for hiding this comment

boopathi Jan 30, 2023

Choose a reason for hiding this comment

oporkka Jan 31, 2023

Choose a reason for hiding this comment

oporkka Jan 31, 2023

Choose a reason for hiding this comment

igrlk commented Jan 31, 2023

igrlk commented Feb 1, 2023

oporkka Feb 2, 2023

Choose a reason for hiding this comment

igrlk Feb 2, 2023

Choose a reason for hiding this comment

oporkka commented Feb 2, 2023

igrlk commented Mar 10, 2023

boopathi commented Mar 13, 2023

oporkka commented Mar 13, 2023 • edited Loading

igrlk commented Jan 29, 2023 •

edited

Loading

oporkka commented Mar 13, 2023 •

edited

Loading