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

Possible reference mistake in subscription execution #994

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

Yogu
Copy link
Contributor

@Yogu Yogu commented Oct 17, 2022

I'm a bit confused about MapSourceToResponseEvent in the execution specification for subscriptions. It explains how to map a sourceStream to a responseStream. Instead of specifying the behavior in case sourceStream completes, it talks about what happens when responseStream completes, even though there is no way how responseStream could complete on its own.

Am I right that the specification part just confused sourceStream and responseStream here?

Also, why is the operation called MapSourceToResponseEvent when it maps event streams? Shouldn't it be called MapSourceStreamToResponseEventStream?

@netlify
Copy link

netlify bot commented Oct 17, 2022

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 012a913
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/634d866f2cb33a0008714e39
😎 Deploy Preview https://deploy-preview-994--graphql-spec-draft.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.

@benjie
Copy link
Member

benjie commented Jun 6, 2024

There's no EasyCLA on this PR, so I'm going to close and open it again. I agree with this change.

@benjie benjie closed this Jun 6, 2024
@benjie benjie reopened this Jun 6, 2024
@leebyron leebyron merged commit 497e333 into graphql:main Jun 6, 2024
5 checks passed
@benjie
Copy link
Member

benjie commented Jun 7, 2024

Hi @Yogu thanks for this fix. Just to summarise the discussion last night: this is indeed the right fix, and the other route of closing the streams (when the user unsubscribes, also (optionally) close the source stream) is covered in the prose above the Unsubscribe algorithm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants