-
-
Notifications
You must be signed in to change notification settings - Fork 821
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
Handle errors better in merged schemas #425
Conversation
* Translate paths * Throw errors per field with errors from remote schema
1221fdf
to
1657ac0
Compare
context, | ||
info, | ||
) => { | ||
const fieldName = info.fieldNodes[0].alias |
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.
Maybe call this responseKey
to avoid confusion with info.fieldName
?
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.
Done.
src/stitching/errors.ts
Outdated
import { GraphQLResolveInfo, responsePathAsArray } from 'graphql'; | ||
import { locatedError } from 'graphql/error'; | ||
|
||
const ERROR_SYMBOL = Symbol('Schema Merging Error'); |
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.
Maybe something like subSchemaErrors
as the symbol name?
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.
Done.
src/stitching/errors.ts
Outdated
} | ||
} | ||
return { | ||
childrenErrors, |
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.
I don't think I fully understand the logic, but is there ever a situation where you'd want to return both ownError
and childrenErrors
?
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.
No. I guess I should use algebraic type with key.
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.
Done.
const { ownError, childrenErrors } = getErrorsFromParent(parent, fieldName); | ||
if (ownError) { | ||
throw locatedError( | ||
ownError.message, |
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.
This is not for now I think, but we may want to make sure we can also support additional error fields besides message.
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.
So the problem is the implementation of locatedError, it copies all the fields like "location" and "path" that we actually want to recreate.
@martijnwalraven All fixed. |
TODO: