Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Add complete handler to Subscription, to handle the case when the ser… #2716

Merged
merged 10 commits into from
Jan 24, 2019

Conversation

sujeetsr
Copy link
Contributor

@sujeetsr sujeetsr commented Jan 7, 2019

Handle finite subscriptions
(apollographql/subscriptions-transport-ws#471)

Depends on
apollographql/apollo-client#4290

@hwillson - please review.

@sujeetsr
Copy link
Contributor Author

sujeetsr commented Jan 7, 2019

Sample client and server app to test this: https://github.com/sujeetsr/subscriptions-example

@hwillson hwillson self-assigned this Jan 9, 2019
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks for this @sujeetsr! Just a few small changes, then we should be pretty close here. If you're up for adding a test, similar to

it('calls onSubscriptionData if given', done => {
jest.useFakeTimers();
let count = 0;
const Component = () => (
<Subscription
subscription={subscription}
onSubscriptionData={opts => {
expect(opts.client).toBeInstanceOf(ApolloClient);
const { data } = opts.subscriptionData;
expect(data).toEqual(results[count].result.data);
if (count === 3) done();
count++;
}}
/>
);
wrapper = mount(
<ApolloProvider client={client}>
<Component />
</ApolloProvider>,
);
const interval = setInterval(() => {
link.simulateResult(results[count]);
if (count >= 3) clearInterval(interval);
}, 10);
jest.runTimersToTime(40);
});

that would be great as well. If not, not worries - just let me know and I'll add it in (and add onSubscriptionComplete to the docs). Thanks!

src/Subscriptions.tsx Outdated Show resolved Hide resolved
src/Subscriptions.tsx Outdated Show resolved Hide resolved
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @sujeetsr!

@hwillson hwillson merged commit f95fca7 into apollographql:master Jan 24, 2019
@sujeetsr
Copy link
Contributor Author

@hwillson thanks for merging!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants