Two-subscription test that actually works and fix. #694
Conversation
| @@ -601,7 +601,13 @@ describe('QueryManager', () => { | |||
| handle.refetch(); | |||
| } else if (subTwoCount === 2) { | |||
| assert.deepEqual(result.data, data2); | |||
| done(); | |||
| setTimeout(() => { | |||
There was a problem hiding this comment.
It's not clear which subscriber will trigger first so this timeout is necessary.
|
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 |
|
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:
Are all in fact variants on the same thing. And the listener is created in I need a consult. |
|
Probably related: #703 |
|
@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. |
|
@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. |
|
Ok, well maybe instead we should just cherry pick the tests across into the refactoring. Up to you. |
|
Yeah, +1 for the tests. Although right now they're failing since I merged that other PR :( |
|
Also, calling And thus calling |
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.
stubailo
force-pushed
the
cannot-subscribe-two-times-version-2
branch
from
e1799b0 to
3f5ab67
Compare
|
Fixed in #791 |
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.