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

Fix formatting and typo in MapSourceToResponseEvent #1070

Closed
wants to merge 5 commits into from

Conversation

benjie
Copy link
Member

@benjie benjie commented Dec 14, 2023

Based on #1069; that PR needs merging first (hence this is marked as a draft). If reviewing this, just look at the final commit only.

Lee did an editorial edit to the subscriptions PR ("Spec: Formal Subscriptions Definition") before merging, and added a line (in this commit) to MapSourceToResponseEvent:

 MapSourceToResponseEvent(sourceStream, subscription, schema, variableValues):
 
   * Return a new event stream {responseStream} which yields events as follows:
   * For each {event} on {sourceStream}:
     * Let {response} be the result of running
       {ExecuteSubscriptionEvent(subscription, schema, variableValues, event)}.
     * Yield an event containing {response}.
+  * When {responseStream} completes: complete this event stream.

The "this event stream" is unclear; I would expect it to refer to responseStream (the stream that we've created) but When {responseStream} completes: complete {responseStream}. doesn't make sense.

I figured that it might be a deliberate way of saying that {sourceStream} should be closed if {responseStream} completes, however that's already handled in the Unsubscribe algorithm text:

Unsubscribe

Unsubscribe cancels the Response Stream when a client no longer wishes to
receive payloads for a subscription. This may in turn also cancel the Source
Stream. This is also a good opportunity to clean up any other resources used by
the subscription.

Unsubscribe(responseStream):

  • Cancel {responseStream}.

Note "This may in turn also cancel the Source Stream." - i.e. may, we are not enforcing this. So MapSourceToResponseEvent should not enforce it.

I believe this was a simple typo, and should have been When {sourceStream} completes: complete {responseStream}. so I have made this edit.

Also, we now enforce consistent formatting in the spec (via #1069), so algorithm steps after a : should be indented; I've fixed this also.

Copy link

netlify bot commented Dec 14, 2023

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 8536e49
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/657c2a316e6a5000086a35f5
😎 Deploy Preview https://deploy-preview-1070--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 configuration.

@yaacovCR
Copy link
Contributor

If the line was originally supposed to refer to the source stream, or even if we are simply just changing it to refer to the source stream, we cannot say “when the source stream completes “, we have to say instead “if the source stream completes. “

@benjie
Copy link
Member Author

benjie commented Dec 15, 2023

I don't think it's necessary but nonetheless I've changed it to "if and when" to make clear both the conditional ("if") and timely ("when") elements.

I'd argue thing.on('error', e => console.log(e)) can be expressed as "when an error occurs, log it" since the "when" can introduce the condition whilst also making the "at the time of" explicit; I don't think it matters that the "when" may never occur. However it's arguably more precise (though a little pedantic) to say "if and when an error occurs, log it"; and what is a specification if not pedantically precise? 😆

@benjie
Copy link
Member Author

benjie commented Sep 19, 2024

This was later spotted by @Yogu and their fix has since been merged; so this PR is no longer needed. Ref: #994

@benjie benjie closed this Sep 19, 2024
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.

2 participants