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.

Sign up for GitHub

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

Jump to bottom

Upgrade to graphql 16 #686

Merged
merged 1 commit into from Dec 20, 2021
Merged

Upgrade to graphql 16 #686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
126 changes: 0 additions & 126 deletions examples/graphql-shield-authorization.js
View file
Open in desktop

This file was deleted.

23 changes: 15 additions & 8 deletions index.js
View file
Open in desktop
Expand Up @@ -535,7 +535,14 @@ const plugin = fp(async function (app, opts) {
const shouldCompileJit = cached && cached.count++ === minJit
// Validate variables
if (variables !== undefined && !shouldCompileJit) {
const executionContext = buildExecutionContext(fastifyGraphQl.schema, document, root, context, variables, operationName)
const executionContext = buildExecutionContext({
schema: fastifyGraphQl.schema,
document,
rootValue: root,
contextValue: context,
variableValues: variables,
operationName
})
if (Array.isArray(executionContext)) {
const err = new MER_ERR_GQL_VALIDATION()
err.errors = executionContext
Expand Down Expand Up @@ -566,14 +573,14 @@ const plugin = fp(async function (app, opts) {
return maybeFormatErrors(execution, context)
}

const execution = await execute(
modifiedSchema || fastifyGraphQl.schema,
modifiedDocument || document,
root,
context,
variables,
const execution = await execute({
schema: modifiedSchema || fastifyGraphQl.schema,
document: modifiedDocument || document,
rootValue: root,
contextValue: context,
variableValues: variables,
operationName
)
})

return maybeFormatErrors(execution, context)
}
Expand Down
47 changes: 14 additions & 33 deletions lib/errors.js
View file
Open in desktop
Expand Up @@ -26,39 +26,20 @@ class FederatedError extends Error {
// allows to copy the `path` & `locations` properties
// from the already serialized error
function toGraphQLError (err) {
return Object.create(GraphQLError, {
name: {
value: err.name
},
message: {
value: err.message,
enumerable: true,
writable: true
},
locations: {
value: err.locations || undefined,
enumerable: true
},
path: {
value: err.path || undefined,
enumerable: true
},
nodes: {
value: err.nodes || undefined
},
source: {
value: err.source || undefined
},
positions: {
value: err.positions || undefined
},
originalError: {
value: err.originalError || undefined
},
extensions: {
value: err.extensions || undefined
}
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

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 got this error error.toJSON is not a function

That's because they changed the formatError impl to simply do return error.toJSON();

I thought it was safer to move to new GraphQLError

Should I go back to Object.create?

Doing Object.create(GraphQLError.prototype, would also solve the issue

const gqlError = new GraphQLError(
err.message,
err.nodes,
err.source,
err.positions,
err.path,
err.originalError,
err.extensions
)

gqlError.locations = err.locations
gqlError.name = err.name

return gqlError
}

function defaultErrorFormatter (err, ctx) {
Expand Down
1 change: 1 addition & 0 deletions lib/federation.js
View file
Open in desktop
Expand Up @@ -129,6 +129,7 @@ function getStubTypes (schemaDefinitions, isGateway) {

function gatherDirectives (type) {
let directives = []
/* istanbul ignore else seems that extensionASTNodes is always provided in graphql16+, we are keeping this check for safety */
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this comment doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In graphql 15 extensionASTNodes could be either null or Array

Now it is only Array but I thought that was safer to keep the check.

I've added this comment to get the 100% coverage and explain why I didn't get rid of it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok so this comment mixes an istanbul command with human-readable information.
I would recommend we keep the two separated, meaning, 2 comments

if (type.extensionASTNodes) {
for (const node of type.extensionASTNodes) {
/* istanbul ignore else we are not interested in nodes that does not have directives */
Expand Down
2 changes: 1 addition & 1 deletion lib/gateway/make-resolver.js
View file
Open in desktop
Expand Up @@ -39,7 +39,7 @@ function getDirectiveSelection (node, directiveName) {
}

function getDirectiveRequiresSelection (selections, type) {
if (!type.extensionASTNodes ||
if (!type.extensionASTNodes || type.extensionASTNodes.length === 0 ||
!type.extensionASTNodes[0].fields[0] ||
!type.extensionASTNodes[0].fields[0].directives[0]) {
return []
Expand Down
10 changes: 5 additions & 5 deletions lib/subscription-connection.js
View file
Open in desktop
Expand Up @@ -183,11 +183,11 @@ module.exports = class SubscriptionConnection {
subscriptionLoaders = this.fastify[kSubscriptionFactory]
}

const subIter = await subscribe(
const subIter = await subscribe({
schema,
document,
{}, // rootValue
{
rootValue: {},
contextValue: {
...context,
get __currentQuery () {
return print(document)
Expand All @@ -201,9 +201,9 @@ module.exports = class SubscriptionConnection {
[kEntityResolvers]: this.entityResolvers
}
},
variables,
variableValues: variables,
operationName
)
})
this.subscriptionIters.set(id, subIter)

if (subIter.errors) {
Expand Down
6 changes: 2 additions & 4 deletions package.json
View file
Open in desktop
Expand Up @@ -26,7 +26,7 @@
},
"homepage": "https://mercurius.dev",
"peerDependencies": {
"graphql": "^15.5.1"
"graphql": "^16.0.0"
},
"devDependencies": {
"@graphql-tools/merge": "^8.0.0",
Expand All @@ -41,8 +41,6 @@
"concurrently": "^6.2.0",
"docsify-cli": "^4.4.3",
"fastify": "^3.18.1",
"graphql-middleware": "^6.0.10",
"graphql-shield": "^7.5.0",
"graphql-tools": "^8.0.0",
"pre-commit": "^1.2.2",
"proxyquire": "^2.1.3",
Expand All @@ -62,7 +60,7 @@
"fastify-plugin": "^3.0.0",
"fastify-static": "^4.2.2",
"fastify-websocket": "^4.0.0",
"graphql": "^15.5.1",
"graphql": "^16.0.0",
"graphql-jit": "^0.7.0",
"mqemitter": "^4.4.1",
"p-map": "^4.0.0",
Expand Down
1 change: 0 additions & 1 deletion tap-snapshots/test/federation.js.test.cjs
View file
Open in desktop
Expand Up @@ -43,5 +43,4 @@ type User {
}

union _Entity = Product | User

`
4 changes: 2 additions & 2 deletions test/subscription-connection.js
View file
Open in desktop
Expand Up @@ -295,7 +295,7 @@ test('subscription connection send error message when client message type is inv
}))
})

test('subscription connection handles GQL_START message correctly, when payload.query is not defined', async (t) => {
test('subscription connection handles GQL_START message correctly, when payload.query is not defined', (t) => {
t.plan(1)

const sc = new SubscriptionConnection({
Expand All @@ -311,7 +311,7 @@ test('subscription connection handles GQL_START message correctly, when payload.

sc.isReady = true

await sc.handleMessage(JSON.stringify({
sc.handleMessage(JSON.stringify({
simoneb marked this conversation as resolved.
Show resolved Hide resolved
id: 1,
type: 'start',
payload: { }
Expand Down