Skip to content

Simplify SubscriptionConnection #719

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

Merged
merged 6 commits into from
Jul 29, 2020

Conversation

ccbrown
Copy link
Contributor

@ccbrown ccbrown commented Jul 25, 2020

Previously SubscriptionConnection was a stream of GraphQLResponse<'a, S>, a newtype for:

Result<(Value<S>, Vec<ExecutionError<S>>), GraphQLError<'a>>

This doesn't really make much sense because subscription events cannot have any sort of pre-execution errors and the GraphQLResponse is always Ok. This is a bit misleading and complicates lifetimes as the output of the stream can otherwise be 'static.

This PR just makes SubscriptionConnection a stream of unwrapped tuples. This has the added benefit of allowing the user of the API to actually access the data / errors before serializing. Alternatively, it might make sense to create a new type for this, e.g. an ExecutionResult type.

This is something I came across while working on #709 that I figured could go in its own PR.

@LegNeato
Copy link
Member

ExecutionResult sounds like a good idea. In general, I personally prefer to use explicit types rather than tuples where both sides would used a similar number of times...but it isn't a requirement.

@ccbrown
Copy link
Contributor Author

ccbrown commented Jul 29, 2020

Sounds good, will update shortly.

@ccbrown ccbrown force-pushed the simplify-subscription-conn branch from edfea63 to 79e4c3b Compare July 29, 2020 02:12
@ccbrown
Copy link
Contributor Author

ccbrown commented Jul 29, 2020

@LegNeato Updated. Went with ExecutionOutput since it's not a Result type and ExecutionResult already exists.

Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks! 🍻

@LegNeato LegNeato merged commit dc309b8 into graphql-rust:master Jul 29, 2020
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