fix resetStore running stopped queries

[machard]

Hi,

there is an issue actually when stoping a query and then resetting the store.
the query will actually be refetched.

It seems to be a possible logical fix. but it fails tests

[apollo-cla]

@machard: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[yurigenin]

This will have a side effect of breaking logic of calling updateQueries on a mutation for this deleted observable query.

[machard]

Ok, so what about updating ObservableQuery.refetch method to return early if the ObservableQuery has no subscribers ?

I also don't understand why we do a refetch (forceFetch https://github.com/apollostack/apollo-client/blob/master/src/core/ObservableQuery.ts#L163) on resetStore as calling resetStore means "empty the store" as i understand it. A possible other solution would be to do a noFetch refresh of the observable queries when resetting the store ?

[machard]

ok after reading http://dev.apollodata.com/react/auth.html#login-logout, i see that it's intentional to reset the store as it is on the server so none of the 2 methods in the previous comment could be a match.
But it does seem like a real problem that teardowned queries are refetch on reset store.

[tmeasday]

I think it is pretty reasonable to not refetch the query if it has been torn down on resetting store. @helfer what do you think?

[helfer]

@tmeasday Yes, the query definitely shouldn't be refetched if it has been stopped.

@machard I think the proper fix for now would be to only refetch non-stopped queries here.

[machard]

is there already a way to know for sure if a request has been toredown (observers.length == 0 ?) or should i add a toredown property to the observableQuery ?

[machard]

@helfer ?

[helfer]

@machard I'm not sure what you mean. A query isn't started until it has listeners, and when it has no more listeners, it is stopped. What I proposed was checking the stopped property in the store before refetching.

[machard]

@helfer yes you answered my question. I updated the PR, is that what you expected ?

[helfer]

@machard yes, thanks a lot, that's exactly what I meant! It would be awesome if you could add a test for this as well that makes sure stopped queries are really not refetched. Do you think you could do that? Basically it should start a query, then stop it, then call refetch store and make sure no network request is sent. To make sure the test actually works, you could try and see if the test fails when you remove your changes, and passes when you add them back in.

Let me know if you can do it!?

[machard]

@helfer done!

[helfer]

Thanks a lot, it looks great! 🙂

I'm going to merge it now, and then make the timeout a bit shorter because nobody wants to wait 400 ms for a test to finish 😉

[helfer]

And I guess I'll update the changelog. I didn't even notice that you deleted the todo list that comes with the PR template. That's not what it's for 😄

[machard]

cool !
for the timeout, I took the one i saw on other tests :/ https://github.com/apollostack/apollo-client/blob/master/test/QueryManager.ts#L2234 for example

[helfer]

@machard Oh, right. I should probably go and reduce those where possible to make tests run faster.