fix resetStore / skip logic

[machard]

When skip is true, then switch back to false, if we call resetStore, it will run the query.
This fix propose to tearDown the queryObservable instead of just unsubscribing.
The queryObservable is already recreated if it's missing

Need apollographql/apollo-client#960

see #289 (comment)

[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/

[tmeasday]

I think we need to start with your issue/PR against AC and figure out if tearDownQuery makes sense (it probably does, although I wonder if it should be the observableQuery that reference counts it's # of subscribers).

[tmeasday]

(If we do my approach then no PR to RA will be necessary).

[machard]

ok, so i did the AC pr in your repo :)

apollographql/apollo-client#960

[machard]

Ok so I digged a little bit more. As of now on master:

1. when all subscribers are removed the query is actually already teardowned: https://github.com/apollostack/apollo-client/blob/master/src/core/ObservableQuery.ts#L371

2. having skip switching back to false causes a rerender without data which does not seem to be a wanted behavior, is there a reason? Look at my use case which seems a common possible use of skip :
   skip: ({ userState, isFocused }) => !utils.isReady(userState) || !isFocused,
   as you see, when my component is not focused, it skips the query, when it gets the focus it does the query, when it looses focus i don't want it to rerender without the data already in the store!

I didn't find a proper solution for apollographql/apollo-client#960.
(=> which actually lead to an other problem : all unmounted components will have their queries refetched on resetStore. https://github.com/apollostack/react-apollo/blob/master/src/graphql.tsx#L299)

So I updated this PR with switching the subscribe/unsubscribe behavior of the skip option to set/unset noFetch to at least have a workaround for 1) and 2). tell me what you thing @tmeasday

[tmeasday]

@machard I still don't really know if I understand what this PR is trying to do. I think it would make sense to include a failing test in any case.

[tmeasday]

I still think that some version of apollographql/apollo-client#960 is the way to go here

[machard]

hehe I was trying to fix 2 problems i noticed:

• when a query is supposed to be skipped, resetting the store executes the query against the server. if we find a solution on AC, it will be fixed
• when skip goes from true to false, the component is rerendered without any data! Once the AC issue is solved, this can be an other pr on RA.

This PR is now just a workaround to quickly fix both my issues without needing to change AC.

[tmeasday]

when skip goes from true to false, the component is rerendered without any data! Once the AC issue is solved, this can be an other pr on RA.

Let's take a look at that once the AC issue is resolved

[helfer]

@machard is this still relevant now? If just part of it is, then I would suggest opening a new PR.

[machard]

@helfer yes i am going to make the pr for "when skip goes from true to false, the component is rerendered without any data" this week. i guess you can close this one :)

[machard]

@helfer
Actually to make my new PR without non straightfoward modifications, I need this in AC : apollographql/apollo-client#1067