Add wrapping error and move stacktrace to extensions #272
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. Latest deployment of this branch, based on commit c23444e:
|
| @@ -383,7 +385,7 @@ export async function executeOperation({ | |||
| ? { | |||
| message: err.message, | |||
| ...('stack' in err && typeof err.stack === 'string' | |||
| ? { stack: err.stack } | |||
| ? { extensions: { stack: err.stack } } | |||
There was a problem hiding this comment.
This is currently not GraphQL spec compliant, so moving to extensions fixes that
🦋 Changeset detectedLatest commit: c23444e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
nice, I think it's working?
would it be okay wait for @mayakoneval to get back next week to take another look 🙏 (been a while since I've been in this codebase)
|
I have remove the |
| @@ -383,7 +385,7 @@ export async function executeOperation({ | |||
| ? { | |||
| message: err.message, | |||
| ...('stack' in err && typeof err.stack === 'string' | |||
There was a problem hiding this comment.
If the error stack should be in extensions, is this wrong? and this?
I can't find where the extensions wrapper comes in in the multipart response spec here.
If you test this and errors work, that works for me.
Did we test with defer & subscriptions?
There was a problem hiding this comment.
Yes, those are incorrect too then.
The multipart spec has more wording around the streaming and chunking bits. It just says to follow the GraphQL spec for the other types
Both types of errors will follow the GraphQL error format (but top-level errors will never have locations or path).
That spec states that the GraphQL Error type should only have 4 fields, message, path, locations, and extensions. Any other field (like stack) is discouraged and should instead live under extensions
Previous versions of this spec did not describe the extensions entry for error formatting. While non-specified entries are not violations, they are still discouraged.
There was a problem hiding this comment.
curious if we were able to test that the error messages are getting through as expected w stack. Can we update this PR to edit the multipart & subscription types to wrap in extension?
There was a problem hiding this comment.
Stack message still pass through as expected today, but we are technically not compliant
|
going to test manually before merge and then I will merge myself |
|
@mayakoneval Is this something we can still merge? |
To provide at least a little more info, wrap the fetch function with a message to indicate that we had an issue calling the url, rather than is ambiguous
TypeErrorthat we get today