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

Two-subscription test that actually works and fix. #694

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@tmeasday
Copy link
Collaborator

tmeasday commented Sep 22, 2016

Sigh. The previous test, despite my best efforts, wasn't actually working properly. I think this one does.

The bug is that if you subscribe twice to a query and unsubscribe from one, it also unsubscribes the other.

@@ -601,7 +601,13 @@ describe('QueryManager', () => {
handle.refetch();
} else if (subTwoCount === 2) {
assert.deepEqual(result.data, data2);
done();
setTimeout(() => {

This comment has been minimized.

@tmeasday

tmeasday Sep 22, 2016

Author Collaborator

It's not clear which subscriber will trigger first so this timeout is necessary.

@tmeasday

This comment has been minimized.

Copy link
Collaborator Author

tmeasday commented Sep 23, 2016

Oh, you know what, if you stop one subscription things will break as well: https://github.com/apollostack/apollo-client/blob/master/src/ObservableQuery.ts#L65-L66

Will add to this PR

@tmeasday

This comment has been minimized.

Copy link
Collaborator Author

tmeasday commented Sep 26, 2016

Ok, I realised this was only half the story, and in fact you can't unsubscribe to a single sub either.

I'd like to fix it but the code for tracking this stuff is a complete mess:

  • queryManager.addObservableQuery
  • queryManager.addQuerySubscription
  • queryManager.startQuery

Are all in fact variants on the same thing. And the listener is created in QueryManager.ts yet the unsub function is created in ObservableQuery.ts. Uggh.

I need a consult.

@tmeasday

This comment has been minimized.

Copy link
Collaborator Author

tmeasday commented Sep 27, 2016

Probably related: #703

@tmeasday

This comment has been minimized.

Copy link
Collaborator Author

tmeasday commented Sep 29, 2016

@helfer I'm not sure exactly when I am going to look at this, (maybe next week?) so if you want to merge of it apart from the failing test or something, that might make sense.

At the moment I think you can subscribe twice OK now, you just can't subscribe twice then unsubscribe once right now.

@helfer

This comment has been minimized.

Copy link
Member

helfer commented Sep 29, 2016

@tmeasday yeah, I think it might make more sense for us to clean up the code and fix it during the upcoming refactoring. I'm trying to merge everything I can today so we can refactor without worrying too much.

@tmeasday

This comment has been minimized.

Copy link
Collaborator Author

tmeasday commented Sep 29, 2016

Ok, well maybe instead we should just cherry pick the tests across into the refactoring. Up to you.

@helfer

This comment has been minimized.

Copy link
Member

helfer commented Sep 29, 2016

Yeah, +1 for the tests. Although right now they're failing since I merged that other PR :(

@glasser

This comment has been minimized.

Copy link
Contributor

glasser commented Oct 13, 2016

Also, calling result() on an ObservableQuery can kill its subscription due to this.

And thus calling setVariables or setOptions can also unsubscribe (since in some cases they call result).

glasser added a commit that referenced this pull request Oct 13, 2016

Temporary hack (until #694) to fix setOptions
react-apollo calls setOptions and ignores the return value. Sometimes
setOptions calls this.result() which subscribes and unsubscribes to the
query, but this actually unsubscribes the main subscription too, which
is bad.  We should fix the underlying bug but until then react-apollo
can call _setOptionsNoResult instead.

tmeasday added a commit to apollographql/react-apollo that referenced this pull request Oct 14, 2016

@stubailo stubailo modified the milestone: Release 0.5 Oct 16, 2016

@stubailo stubailo force-pushed the cannot-subscribe-two-times-version-2 branch from e1799b0 to 3f5ab67 Oct 16, 2016

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Oct 17, 2016

Fixed in #791

@stubailo stubailo closed this Oct 17, 2016

@stubailo stubailo deleted the cannot-subscribe-two-times-version-2 branch Jan 5, 2017

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