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

Completed subscription not reflected in hook data #407

Closed
zenios opened this issue Aug 24, 2019 · 7 comments · Fixed by #410
Closed

Completed subscription not reflected in hook data #407

zenios opened this issue Aug 24, 2019 · 7 comments · Fixed by #410

Comments

@zenios
Copy link

zenios commented Aug 24, 2019

There are two cases where a subscription query completed. Either it unmounts or the server completes the source. i was expecting that on source completion the "fetching" value of useSubscription would turn to false but it still stays the same.

I went through the code of the subscription exchange and found out that the complete event from forwardSubscription is sent to the wonka stream via the complete function but for an unknown reason it does not propagate up to the parent.

@zenios zenios added the bug 🐛 Oh no! A bug or unintented behaviour. label Aug 24, 2019
@kitten kitten removed the bug 🐛 Oh no! A bug or unintented behaviour. label Aug 24, 2019
@kitten
Copy link
Member

kitten commented Aug 24, 2019

Hiya 👋 this is a known limitation since we weren't sure whether it was worth forwarding completed subscriptions through to the hook and exchanges.

It's possible to customise the data coming back to add this for now with a slightly customised exchange.

If you're interested the line in question is here: https://github.com/FormidableLabs/urql/blob/master/src/exchanges/subscription.ts#L78

We basically have no result that corresponds to the completed subscription. The useSubscription hook still has a fetching flag which wasn't on purpose actually, but do you know of any use case where this would be useful? Happy to reconsider and come up with a solution 👍

@kitten kitten changed the title Subscription always fetching Completed subscription not reflected in hook data Aug 24, 2019
@zenios
Copy link
Author

zenios commented Aug 24, 2019 via email

@JoviDeCroock
Copy link
Collaborator

I've been considering this, I'm not entirely sure if the completion addition will be worth it, that aside I agree that fetching on the hook can be quite confusing since it can be considered an indicator that the subscription is running.

I don't think leaving fetching out is a breaking change (please correct me if I'm wrong) since most people at this moment rely on the callback to add the event somewhere or to trigger something. I can't see a use case to rely on the current state of the fetching variable.

@kitten
Copy link
Member

kitten commented Aug 25, 2019

I think we could also add it by sending a teardown as a reexecuted operation on completion. Thst being said that would also need some minor hook changes

@JoviDeCroock
Copy link
Collaborator

Then we need to make a distinction between a "completed" teardown and a connection interrupted right?

@kitten
Copy link
Member

kitten commented Aug 26, 2019

This will be shipped in the next release, which will probably be a quick patch release 🎉

We've accomplished this—as was planned above—by reusing the teardown signal. The teardown signal is used to cancel ongoing operations in the exchange chain. It is sent automatically when all components/hooks that are interested in one operation have unsubscribed/unmounted and aren't listening to results anymore.

The change is to make teardown an active operation signal as well. When it's now sent by an exchange (or manually of course) it actively cancels ongoing result streams, which the hooks can use to set fetching to false.

@zenios
Copy link
Author

zenios commented Aug 26, 2019 via email

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 a pull request may close this issue.

3 participants