-
Notifications
You must be signed in to change notification settings - Fork 259
Improve error handling on the supergraph #1586
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
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Some errors might be thrown while extracting subgraphs from the supergraph but without being handled gracefully, resulting in messages to the user that were less useful than they should be.
df33767
to
c6c61bd
Compare
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.
LGTM
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.
🐐
@@ -292,11 +292,11 @@ export function extractSubgraphsFromSupergraph(supergraph: Schema): Subgraphs { | |||
if (isFed1) { | |||
// Note that this could be a bug with the code handling fed1 as well, but it's more helpful to ask users to recompose their subgraphs with fed2 as either | |||
// it'll solve the issue and that's good, or we'll hit the other message anyway. | |||
const msg = `Error extracting subgraph ${subgraph.name} from the supergraph: this might due to errors in subgraphs that were mistakenly ignored by federation 0.x versions but are rejected by federation 2.\n` | |||
+ 'Please try composing your subgraphs with federation 2: this should help precisely pinpoint the errors and generate a correct federation 2 supergraph.'; | |||
const msg = `Error extracting subgraph "${subgraph.name}" from the supergraph: this might due to errors in subgraphs that were mistakenly ignored by federation 0.x versions but are rejected by federation 2.\n` |
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.
change "this might due" to "this might be due"
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. Thanks for noticing.
throw new Error(`${msg}.\n\nDetails:\n${errorToString(e)}`); | ||
} else { | ||
const msg = `Unexpected error extracting subgraph ${subgraph.name} from the supergraph: this is either a bug, or the supergraph has been corrupted.`; | ||
const msg = `Unexpected error extracting subgraph ${subgraph.name} from the supergraph: this is either a bug, or the supergraph has been corrupted`; |
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.
what should folks do as a next step if they hit this error 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.
Most likely open a ticket. The only reason I didn't wrote as much is that this can be throw due to someone manually messing up with the supergraph too, so I wanted to force users to switch their system 2 before opening a ticket. But I don't mind being more explicit.
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.
wfm, that's about what I thought as well. I'm cool with leaving this as is and if we need to change it later, we can.
16d30b7
to
4b3f7dc
Compare
Some errors might be thrown while extracting subgraphs from the
supergraph but without being handled gracefully, resulting in messages
to the user that were less useful than they should be.