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

Never fetch recycled queries on network interface #531

Merged
merged 14 commits into from Apr 7, 2017

Conversation

Projects
None yet
4 participants
@jacobk
Copy link
Contributor

jacobk commented Mar 13, 2017

@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Mar 13, 2017

@jacobk: 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/

calebmer added some commits Mar 13, 2017

@calebmer
Copy link
Contributor

calebmer left a comment

Looks great 👍

I’d just like to see a few changes to increase test completeness 😊

@@ -636,6 +636,9 @@ class ObservableQueryRecycler {
// Stop the query from polling when we recycle. Polling may resume when we
// reuse it and call `setOptions`.
observableQuery.stopPolling();
observableQuery.setOptions({
fetchPolicy: 'cache-only',

This comment has been minimized.

@calebmer

This comment has been minimized.

@jacobk

jacobk Mar 13, 2017

Author Contributor

Fixed

</ApolloProvider>
);

expect(Object.keys((client as any).queryManager.observableQueries)).toEqual(['1']);

This comment has been minimized.

@calebmer

calebmer Mar 13, 2017

Contributor

Could we check that this observable query still exists after we unmount as well?

This comment has been minimized.

@jacobk

jacobk Mar 13, 2017

Author Contributor

Fixed

client.resetStore();

// The query should only have been invoked when first mounting and not when resetting store
expect(networkInterface.query).toHaveBeenCalledTimes(1);

This comment has been minimized.

@calebmer

calebmer Mar 13, 2017

Contributor

Let’s also check that this method was called once before we unmounted.

You should also duplicate this test and assert that query is called twice if we do not unmount, but we still call resetStore.

This comment has been minimized.

@jacobk

jacobk Mar 13, 2017

Author Contributor

Fixed

Jacob Kristhammar added some commits Mar 13, 2017

calebmer added some commits Mar 17, 2017

@calebmer
Copy link
Contributor

calebmer left a comment

I thought of another edge case.

@@ -635,7 +635,10 @@ class ObservableQueryRecycler {
public recycle (observableQuery: ObservableQuery<any>): void {
// Stop the query from polling when we recycle. Polling may resume when we
// reuse it and call `setOptions`.
observableQuery.stopPolling();
observableQuery.setOptions({
fetchPolicy: 'cache-only',

This comment has been minimized.

@calebmer

calebmer Mar 17, 2017

Contributor

On second thought, I’m not ready to merge this because if an observable query gets reused the fetchPolicy will stay as cache-only which is undesirable. Could you write a test to show this doesn’t happen? If it does could you make sure it does not?

This comment has been minimized.

@calebmer

calebmer Mar 17, 2017

Contributor

I think the solution is to update line 672 so that it sets the default fetch policy if no other fetch policy was found. So:

observableQuery.setOptions({
  ...options,
  fetchPolicy: options.fetchPolicy || 'xyz',
});

Where 'xyz' is the default fetchPolicy. This should guarantee that the cache-only policy is reset.

This comment has been minimized.

@jacobk

jacobk Mar 17, 2017

Author Contributor

Sure!
I thought that setOptions where strictly overriding the old options but I just noticed that it doesn't. As far as I can tell there's no explicit default for fetchPolicy and the current setOptions implementation doesn't allow removing previously set properties:

    const oldOptions = this.options;
    this.options = {
      ...this.options,
      ...opts,
    } as WatchQueryOptions;

Ideally I'd like to remove both the pollInterval and fetchPolicy completely before applying options inside reuse.

Any ideas? I'll commit the new breaking test case for now (which is testing for strict equality of options before and after recycle).

This comment has been minimized.

@jacobk

jacobk Mar 17, 2017

Author Contributor

I've pushed a possible fix for reverting the options changed when recycling. IMHO it's a little on the hacky side since I need to manually overwrite the ObservableQuery's options to fully remove the altered options.

Maybe setOptions should be updated to allow for options to be removed completely?

This comment has been minimized.

@calebmer

calebmer Mar 17, 2017

Contributor

Let’s try instead saving the old options in recycle() and then merging those options with the new options in reuse(). Do you think that would work?

But perhaps this isn’t the right thing to do at all. Perhaps what we really need to do is create some kind of light-weight subscribe in apollo-client that will only allow receiving updateQueries and refetchQueries instead of writing workarounds in the recycler…

This comment has been minimized.

@jacobk

jacobk Mar 17, 2017

Author Contributor

The issue with merging options is that you cannot remove any - only update and add. I think fetchPolicy is omitted by default. Maybe it's enough to set it to undefined instead? But then it's a less straight forward procedure to store the old options and later restore them if the values needs to be reasoned about.

But if it's a sound approach to simply override the observableQ.options before calling setOptions (like I do now, that could be done with the snapshotted options too of course).

I haven't studied the whole thing to fully reason about the recycler. However, the current issue with fetchPolicy and refetching onresetStore is giving us real trouble. Given the issue at hand the limitations of apollo-client's setOptions are my only concern.

This comment has been minimized.

@calebmer

calebmer Mar 17, 2017

Contributor

Setting to undefined should be the equivalent of removing. Does this make the implementation easier?

I don’t like the idea of setting options manually mainly because we haven’t yet considered all of the implications 😣

@jacobk

This comment has been minimized.

Copy link
Contributor Author

jacobk commented Mar 17, 2017

The last commit sets the altered options to undefined instead. I think this is symmetric enough compared to keeping a copy of the old options around and still do the same manual handling of the options that should be set to undefined if they weren't initially set.

Jacob Kristhammar
@jacobk

This comment has been minimized.

Copy link
Contributor Author

jacobk commented Mar 27, 2017

@calebmer happy with last version or do you have doubts about this approach?

jacobk added some commits Mar 30, 2017

@jacobk

This comment has been minimized.

Copy link
Contributor Author

jacobk commented Apr 1, 2017

@helfer, any thoughts on this PR, anything else needs changing? (@calebmer asked me to poke you in is absence)

@helfer

This comment has been minimized.

Copy link
Member

helfer commented Apr 1, 2017

Thanks @jacobk! I'm away for the weekend, but I'll take a look on Monday. 🙂

@jacobk

This comment has been minimized.

Copy link
Contributor Author

jacobk commented Apr 1, 2017

Great, thanks!

@helfer

This comment has been minimized.

Copy link
Member

helfer commented Apr 7, 2017

Thanks @jacobk, I think this looks good now, and it might fix the resetStore issue, so I'll release a new patch version with this change before the weekend! 🙂

@helfer

helfer approved these changes Apr 7, 2017

Copy link
Member

helfer left a comment

ship it.

@helfer helfer merged commit ed456a2 into apollographql:master Apr 7, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 92.334%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment