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

V1 subscriptions #160

Merged
merged 2 commits into from Jan 18, 2019

Conversation

Projects
None yet
3 participants
@blainekasten
Copy link

blainekasten commented Dec 28, 2018

Adds Subscriptions to V1

This API is mostly inline with v0 with a few changes.

  1. transformExchange becomes forwardSubscription(operation, observable): { unsubscribe: () => void} in ClientOptions.
  2. Connect takes an array of subscriptions instead of just one.
  3. Changes the updateSubscription callback fn to have a type parameter as it's first argument, which is the name of the subscription (Allowing the user to differ between multiple subscription responses).

How to use:

import { createClient } from 'urql';
import { SubscriptionClient } from 'subscriptions-transport-ws';

const subscriptionClient = new SubscriptionClient('ws://localhost:3000', {});

const client = createClient({
  url: 'http://localhost:3001/graphql',
  forwardSubscription(operation, observer) {
    return subscriptionClient.request(operation).subscribe({
      next(data) {
        observer.next({ operation, data, error: null });
      },
      error(error) {
        observer.error({ operation, data: null, error });
      },
    });
  }
});

Internally

Internally a few notable things are happening

  1. Exchanges are given clientOptions as a new key in the argument.
  2. ClientInstance gets an executeSubscription method
  3. ClientInstance gets an executeUnsubscribeSubscription method
  4. ClientInstance configuration has a new callback method specifically for subscriptions: onSubscriptionUpdate
  5. a new exchange is created for subscriptions
  6. operationName internally are switched from a string to an enum

other stuff

  1. Backend is updated to support subscriptions

@blainekasten blainekasten changed the base branch from master to v1 Dec 28, 2018

@blainekasten blainekasten force-pushed the v1-subscriptions branch from 9f50b7f to f524c78 Dec 28, 2018

@blainekasten blainekasten force-pushed the v1-subscriptions branch 4 times, most recently from a263bee to b5f8c6b Dec 29, 2018

@blainekasten

This comment has been minimized.

Copy link

blainekasten commented Dec 31, 2018

Just pushed a big change to handle subscription prop changes and unmounting to properly dispose of subscription connections.

@blainekasten blainekasten force-pushed the v1-subscriptions branch 7 times, most recently from 967a4be to d2bb5f0 Dec 31, 2018

@blainekasten blainekasten changed the title [WIP] V1 subscriptions V1 subscriptions Jan 2, 2019

@blainekasten blainekasten force-pushed the v1-subscriptions branch 2 times, most recently from cdd0c79 to 834b710 Jan 3, 2019

@andyrichardson

This comment has been minimized.

Copy link

andyrichardson commented Jan 3, 2019

I like the forwardSubscription idea, To simplify things and remove the need for mutation, can we do something like this?

createClient({
  url: 'http://localhost:3001/graphql',
  onSubscription: (payload: string) => subscriptionClient.request(payload),
});

This way the user can return an observable/subject for the graphql request.

I would have some questions about this approach:

  • Your example looks to use a new connection for each subscription query. Is that the case, and if so, is that the common practice for most graphql subscription/websocket clients?
  • Are we handling connection states such as an the closing of a connection?
Show resolved Hide resolved src/types.ts Outdated
Show resolved Hide resolved src/lib/typenames.ts Outdated
Show resolved Hide resolved src/lib/client.ts Outdated
Show resolved Hide resolved src/types.ts
@blainekasten

This comment has been minimized.

Copy link

blainekasten commented Jan 3, 2019

I like the forwardSubscription idea, To simplify things and remove the need for mutation, can we do something like this?

createClient({
  url: 'http://localhost:3001/graphql',
  onSubscription: (payload: string) => subscriptionClient.request(payload),
});

This way the user can return an observable/subject for the graphql request.

I'm not sure that simplifies anything actually. In reality, I need the response to include the unsubscribe method so I can remove graphql subscription connections when a subscription unmounts or changes.

I would have some questions about this approach:

Your example looks to use a new connection for each subscription query. Is that the case, and if so, is that the common practice for most graphql subscription/websocket clients?
Are we handling connection states such as an the closing of a connection?

Actually it's doing the opposite. When you init the subscriptionClient that is what creates the websocket connection. Internally, every time you run executeSubscription it sends a frame through the ws connection for the backend to know about:

image

Then, when you run executeUnsubscribeSubscription it sends new frames to tell the backend they have stopped:

image

@blainekasten blainekasten force-pushed the v1-subscriptions branch 5 times, most recently from 8f06085 to 8771012 Jan 4, 2019

@blainekasten blainekasten force-pushed the v1-subscriptions branch from 8771012 to 4551fe0 Jan 4, 2019

Add Subscription query types to v1
Handle subscription prop changes

Update API

documentation and tests

migration
@@ -11,6 +11,10 @@ import { createClient, createQuery } from 'urql';
const client = createClient({
url: 'https://my-host/graphql',
forwardSubscription(operation, observer) => {

This comment has been minimized.

@kitten

kitten Jan 5, 2019

Member

I'm not sure whether it makes sense to have options like this on the client. Wouldn't these options be mostly forwarded to exchanges' factory functions? It might be worth to keep as many exchanges as possible opt-in only

unsubscribe: true,
});

// remove cached rx subscription

This comment has been minimized.

@kitten

kitten Jan 5, 2019

Member

Some indentation weirdness?


activateSubscriptions = () => {
(this.props.subscriptions || []).forEach(sub =>
this.state.client.executeSubscription(sub)

This comment has been minimized.

@kitten

kitten Jan 5, 2019

Member

I find it weird that every single subscription is handled separately here. I think the more idiomatic Rx approach would be to merge them all here in the component into a single subscription / observable

@andyrichardson

This comment has been minimized.

Copy link

andyrichardson commented Jan 6, 2019

I need the response to include the unsubscribe method

@blainekasten the suggestion does just that - the only difference is that you're allowing the user to pass the observable to the client rather than getting them to mutate an existing subject created by the client.

It would look something like this in urql:

// Call client provided function
const subscription = opts.onSubscription(query).subscribe(
  next,
  error,
  complete,
);

// At unsubscribe
subscription.unsubscribe();

It's exactly the same as what you're proposing but without the need for mutation.


Actually it's doing the opposite.

Sorry, this is my lack of understanding... If we use a single connection (which makes sense), when we receive a response, how do we determine which subscription operation the response is for?

Edit: Just read your comment - I guess we'll have to work this out. A new connection per subscription would work but probably isn't the most optimal method 😄

@kitten

This comment has been minimized.

Copy link
Member

kitten commented Jan 16, 2019

@blainekasten as discussed this is ready to go and only needs some minor touch ups. However, due to the observable refactor it'll need some changes. Let me know if I should quickly make those changes or not :)

@kitten kitten merged commit cbaa747 into v1 Jan 18, 2019

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
security/snyk - package.json (kitten) No new, high severity issues
Details

@kitten kitten deleted the v1-subscriptions branch Jan 18, 2019

@kitten kitten referenced this pull request Jan 21, 2019

Merged

V1 Replace RxJS with Wonka #166

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment