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

Add option to let subscriptionExchange handle all operations #544

Merged
merged 3 commits into from
Feb 14, 2020

Conversation

kitten
Copy link
Member

@kitten kitten commented Feb 14, 2020

This adds an option to the subscriptionExchange to let it handle query and mutation operations as well.

Resolve #543

@kitten kitten requested a review from JoviDeCroock February 14, 2020 14:04
@changeset-bot
Copy link

changeset-bot bot commented Feb 14, 2020

🦋 Changeset is good to go

Latest commit: 73bdd69

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

const isSubscriptionOperation = (operation: Operation) =>
operation.operationName === 'subscription';
/** This flag may be turned on to allow your subscriptions-transport to handle all operation types */
enableAllOperations?: boolean;
Copy link

Choose a reason for hiding this comment

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

Why don't we use filter function instead? There might be cases (I cannot think of any) where someone wants to pass only subscriptions and mutation via subscription exchange.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it makes sense to anticipate use-cases that we can't imagine. To a certain extent we tell people that "forking" our exchanges or making more specific ones is fine.

So overall we only implement what we can see as a use-case right now, instead of bloating our API surface 👍 Hope that makes sense to you though

@zenios
Copy link

zenios commented Feb 14, 2020

Also it would be nice if we could pass fetchOptions as well. Right now i am using it to pass an authentication header

@kitten
Copy link
Member Author

kitten commented Feb 14, 2020

@zenios Kind of unrelated, but I think that's already possible.

https://github.com/FormidableLabs/urql/blob/73bdd698f72a17840abcc4f38708fda23d1aaee2/packages/core/src/exchanges/subscription.ts#L46-L48

You receive the entire the masked operation with context in forwardSubscription, so you can just use operation.context.fetchOptions

@zenios
Copy link

zenios commented Feb 14, 2020

@zenios Kind of unrelated, but I think that's already possible.

https://github.com/FormidableLabs/urql/blob/73bdd698f72a17840abcc4f38708fda23d1aaee2/packages/core/src/exchanges/subscription.ts#L46-L48

You receive the entire the masked operation with context in forwardSubscription, so you can just use operation.context.fetchOptions

Not if fetchOptions is a function

@kitten
Copy link
Member Author

kitten commented Feb 14, 2020

@JoviDeCroock JoviDeCroock merged commit 0c5c1fd into master Feb 14, 2020
@JoviDeCroock JoviDeCroock deleted the feat/subscriptions-exchange-for-all-ops branch February 14, 2020 14:28
@zenios
Copy link

zenios commented Feb 14, 2020

@zenios Like in fetchExchange you can just run the function though

https://github.com/FormidableLabs/urql/blob/8877e0fbc1169405e385579934fc68e1daa49942/packages/core/src/exchanges/fetch.ts#L81-L84

If i use this subsciption exchange and fetchOptions is a function on my server fetchOptions will arrive as [object]

@kitten
Copy link
Member Author

kitten commented Feb 14, 2020

@zenios That sounds like a different problem. You just need to call the function. If you're talking about serialisation, we don't plan or account for context ever to be sent to the server, so this shouldn't happen 😅

Either way, if you need an answer to this feel free to open an issue, where we can discuss this separately. 👍

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.

Enhanced subscription exchange
3 participants