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.

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

change incorrect subscribe return type to a GraphQLError #3621

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

yaacovCR
Copy link
Contributor

…rather than a systems error

to match spec (in parallel to field-level resolver errors in execute)

note the addition of the path, as execution has begun

@netlify
Copy link

netlify bot commented May 31, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 277b5fa
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62a1036f5e3ab300080f0b08
😎 Deploy Preview https://deploy-preview-3621--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@yaacovCR yaacovCR added the PR: breaking change 💥 implementation requires increase of "major" version number label May 31, 2022
@yaacovCR yaacovCR requested a review from a team May 31, 2022 12:21
@yaacovCR yaacovCR force-pushed the more-graphql-errors branch from 30f6596 to e8fcbcc Compare June 2, 2022 19:23
Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

I'm for it 👍
Currently, the subscription algorithm doesn't mention this check at all:
https://spec.graphql.org/draft/#sec-Source-Stream

I don't see how this change is contradict spec, so I'm for merging it.

But if you think previous behavior was contradicting the spec it's valuable to document please update the description with links to the spec and more details on what contradiction is.

@yaacovCR Otherwise feel free to merge.

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jun 8, 2022

I think fixing the entire spec in terms of how it clarifies errors is beyond the scope of this PR. See also graphql/graphql-spec#894

And the entire Response section of the spec doesn't take into account subscriptions, so it's tough to say exactly what's going on, but more on that below.

Fundamentally, however, I imagine that the spec purports there to be unified behavior as much as possible between all operations, as within the spec they are all considered different forms of execution, with subscriptions simply a different operation type. That is why this PR, which is one of several on the road toward integrating subscriptions into execute, should require no spec change and actually is more "correct" by the spec.

More specifically, there is a rough equivalence given between ResolveFieldEventStream and ResolveFieldValue and insofar as errors within root field resolvers are field errors for ResolveFieldValue with a path (see your comment, actually, they are presumably field errors also for subscriptions. Just as request errors get a map of just errors for subscriptions, so, too, should root field errors.

In my opinion. :)

…a systems error

to match spec (in parallel to field-level resolver errors in execute)

note the addition of the path, as execution has begun
@yaacovCR yaacovCR force-pushed the more-graphql-errors branch from e8fcbcc to 277b5fa Compare June 8, 2022 20:15
@yaacovCR yaacovCR merged commit ea1894a into graphql:main Jun 8, 2022
@yaacovCR yaacovCR deleted the more-graphql-errors branch June 8, 2022 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: breaking change 💥 implementation requires increase of "major" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants