Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Never fetch recycled queries on network interface #531

Merged
merged 14 commits into from
Apr 7, 2017
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
Expect active development and potentially significant breaking changes in the `0.x` track. We'll try to be diligent about releasing a `1.0` version in a timely fashion (ideally within 1 or 2 months), so that we can take advantage of SemVer to signify breaking changes from that point on.

### vNext
- Make sure recycled queries are in cache only mode so they do not trigger network requests. [PR #531](https://github.com/apollographql/react-apollo/pull/531)

### 1.0.0-rc.1
- Update dependency to Apollo Client 1.0.0-rc.1 [PR #520](https://github.com/apollographql/react-apollo/pull/520)
Expand Down
5 changes: 4 additions & 1 deletion src/graphql.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😣

pollInterval: 0,
});

this.observableQueries.push({
observableQuery,
Expand Down
72 changes: 71 additions & 1 deletion test/react-web/client/graphql/queries.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import * as ReactDOM from 'react-dom';
import * as renderer from 'react-test-renderer';
import { mount } from 'enzyme';
import gql from 'graphql-tag';

import ApolloClient, { ApolloError } from 'apollo-client';
import { NetworkInterface } from 'apollo-client/transport/networkInterface';
import { connect } from 'react-redux';
import { withState } from 'recompose';

Expand Down Expand Up @@ -2176,6 +2176,76 @@ describe('queries', () => {
expect(Object.keys((client as any).queryManager.observableQueries)).toEqual(['1']);
});

it('will not try to refetch recycled `ObservableQuery`s when resetting the client store', () => {
const query = gql`query people { allPeople(first: 1) { people { name } } }`;
const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
const networkInterface = {
query: jest.fn(),
} as NetworkInterface;
const client = new ApolloClient({ networkInterface, addTypename: false });

@graphql(query)
class Container extends React.Component<any, any> {
render () {
return null;
}
}

const wrapper1 = renderer.create(
<ApolloProvider client={client}>
<Container/>
</ApolloProvider>
);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

const queryObservable1 = (client as any).queryManager.observableQueries['1'].observableQuery;

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

wrapper1.unmount();

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

expect(queryObservable1).toBe(queryObservable2);

client.resetStore();

// The query should not have been fetch again
expect(networkInterface.query).toHaveBeenCalledTimes(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

});

it('will refetch active `ObservableQuery`s when resetting the client store', () => {
const query = gql`query people { allPeople(first: 1) { people { name } } }`;
const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
const networkInterface = {
query: jest.fn(),
} as NetworkInterface;
const client = new ApolloClient({ networkInterface, addTypename: false });

@graphql(query)
class Container extends React.Component<any, any> {
render () {
return null;
}
}

const wrapper1 = renderer.create(
<ApolloProvider client={client}>
<Container/>
</ApolloProvider>
);

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

expect(networkInterface.query).toHaveBeenCalledTimes(1);

client.resetStore();

expect(networkInterface.query).toHaveBeenCalledTimes(2);
});

it('will recycle `ObservableQuery`s when re-rendering a portion of the tree', done => {
const query = gql`query people { allPeople(first: 1) { people { name } } }`;
const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
Expand Down