-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Support for graphql@16 #5857
Support for graphql@16 #5857
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,6 @@ | |
"uuid": "^8.0.0" | ||
}, | ||
"peerDependencies": { | ||
"graphql": "^15.3.0" | ||
"graphql": "^15.3.0 || ^16.0.0" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,6 @@ | |
"node": ">=12.0" | ||
}, | ||
"peerDependencies": { | ||
"graphql": "^15.3.0" | ||
"graphql": "^15.3.0 || ^16.0.0" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,21 @@ import { | |
GraphQLFormattedError, | ||
Source, | ||
SourceLocation, | ||
printError, | ||
formatError, | ||
} from 'graphql'; | ||
|
||
declare module 'graphql' { | ||
export interface GraphQLErrorExtensions { | ||
exception?: { | ||
code?: string; | ||
stacktrace?: ReadonlyArray<string>; | ||
}; | ||
} | ||
} | ||
|
||
// Note: We'd like to switch to `extends GraphQLError` and look forward to doing so | ||
// as soon as we drop support for `graphql` bellow `v15.7.0`. | ||
export class ApolloError extends Error implements GraphQLError { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be nice to throw in a comment noting that we'd like to switch to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done ✅ |
||
public extensions: Record<string, any>; | ||
override readonly name!: string; | ||
|
@@ -14,7 +27,7 @@ export class ApolloError extends Error implements GraphQLError { | |
readonly source: Source | undefined; | ||
readonly positions: ReadonlyArray<number> | undefined; | ||
readonly nodes: ReadonlyArray<ASTNode> | undefined; | ||
public originalError: Error | null | undefined; | ||
public originalError: Error | undefined; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To check my understanding: this is a safe change (that won't break users of graphql@15) because it's OK for an implementation of an interface to declare that a field has a type that is a subtype of the supertype's type? Or I guess this is specifically the case because the interface (in v15) declared the field as Right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @glasser Basically, we need to do this change to be compatible with v16. Happy to replace it with some workaround that is less breaking I just can't come up with something right now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah right, I was focused just on "will this work with graphql 15" and not "is this a backwards incompatible change for users". But I think it's fine. We'll make this a minor update at least. |
||
|
||
[key: string]: any; | ||
|
||
|
@@ -40,6 +53,30 @@ export class ApolloError extends Error implements GraphQLError { | |
|
||
this.extensions = { ...extensions, code }; | ||
} | ||
|
||
toJSON(): GraphQLFormattedError { | ||
return formatError(toGraphQLError(this)); | ||
IvanGoncharov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
override toString(): string { | ||
return printError(toGraphQLError(this)); | ||
} | ||
|
||
get [Symbol.toStringTag](): string { | ||
return this.name; | ||
} | ||
} | ||
|
||
function toGraphQLError(error: ApolloError): GraphQLError { | ||
return new GraphQLError( | ||
error.message, | ||
error.nodes, | ||
error.source, | ||
error.positions, | ||
error.path, | ||
error.originalError, | ||
error.extensions, | ||
); | ||
} | ||
|
||
function enrichError(error: Partial<GraphQLError>, debug: boolean = false) { | ||
|
@@ -129,10 +166,13 @@ export function fromGraphQLError(error: GraphQLError, options?: ErrorOptions) { | |
|
||
// copy enumerable keys | ||
Object.entries(error).forEach(([key, value]) => { | ||
if (key === 'extensions') { | ||
return; // extensions are handled bellow | ||
} | ||
copy[key] = value; | ||
}); | ||
|
||
// extensions are non enumerable, so copy them directly | ||
// merge extensions instead of just copying them | ||
IvanGoncharov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
copy.extensions = { | ||
...copy.extensions, | ||
...error.extensions, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,6 @@ | |
}, | ||
"peerDependencies": { | ||
"express": "^4.17.1", | ||
"graphql": "^15.3.0" | ||
"graphql": "^15.3.0 || ^16.0.0" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,6 @@ | |
}, | ||
"peerDependencies": { | ||
"fastify": "^3.17.0", | ||
"graphql": "^15.3.0" | ||
"graphql": "^15.3.0 || ^16.0.0" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,6 @@ | |
}, | ||
"peerDependencies": { | ||
"@hapi/hapi": "^20.1.2", | ||
"graphql": "^15.3.0" | ||
"graphql": "^15.3.0 || ^16.0.0" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,6 @@ | |
"make-fetch-happen": "^8.0.9" | ||
}, | ||
"peerDependencies": { | ||
"graphql": "^15.3.0" | ||
"graphql": "^15.3.0 || ^16.0.0" | ||
} | ||
} |
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.
That was useful.
I discovered a bug in
graphql-js
fortoString
we should followError.toString
behavior so it should beApolloError: My original message
.I will fix it in the
16.0.x
release, I don't think it would be a breaking change for bothgraphql-js
and AS since I just addedGraphQLError.toString
inv16.0.0
.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.
Makes sense. If you're planning to change this in graphql-js, maybe just make this test accept either value? Or we can fix the test in the next graphql-js Renovate PR.