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

feat: add onOperationComplete & onOperationError callbacks #2

Merged
merged 4 commits into from
Jul 23, 2022

Conversation

GNMoseke
Copy link
Contributor

Adds an onComplete callback & registration function for configuring behavior when the server receives a complete message

@GNMoseke
Copy link
Contributor Author

@NeedleInAJayStack This should allow for unsubscribe handling

@GNMoseke
Copy link
Contributor Author

@NeedleInAJayStack Let me know if you think this looks like a more complete system. I added in an onOperationError as well for similar use cases. Same changes in the other repo.

@GNMoseke GNMoseke changed the title fix: add onComplete callback feat: add onOperationComplete & onOperationError callbacks Jul 22, 2022
Comment on lines 21 to 22
var onOperationComplete: () -> Void = {}
var onOperationError: () -> Void = {}
Copy link
Member

Choose a reason for hiding this comment

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

It would be convenient for both of these to provide the ID of the operation that has completed or errored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, added.

@@ -64,6 +66,7 @@ public class Server<InitPayload: Equatable & Codable> {
return
}

// handle incoing message
Copy link
Member

Choose a reason for hiding this comment

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

Fix incoing to incoming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops fixed.

@@ -6,7 +6,7 @@ import GraphQLRxSwift
import NIO
import RxSwift

/// Server implements the server-side portion of the protocol, allowing a few callbacks for customization.
/// Server implements the server-side portion of the protocol, allowing a few callbacks for customization. 0 or 1 subscriptions per connection and no more.
Copy link
Member

Choose a reason for hiding this comment

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

Why do we specify 0 or 1 subscription? I believe it can support any number of subscriptions, as long as they have different IDs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just an incorrect assumption by me that I forgot to remove. Removed now.

@NeedleInAJayStack NeedleInAJayStack self-requested a review July 23, 2022 15:33
Copy link
Member

@NeedleInAJayStack NeedleInAJayStack left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @GNMoseke!

@NeedleInAJayStack NeedleInAJayStack merged commit 37de2c4 into GraphQLSwift:main Jul 23, 2022
NeedleInAJayStack added a commit that referenced this pull request Jan 10, 2025
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