This repository has been archived by the owner on Apr 13, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 786
Never fetch recycled queries on network interface #531
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
9722637
never fetch recycled queries on network interface
9777716
Update queries.test.tsx
calebmer d39b3c1
Update queries.test.tsx
calebmer 82aa79f
only use `setOptions` when recycling `ObservableQuery`s
e1bbde9
Add more tests
278fdce
Merge branch 'master' into master
calebmer 2190a23
Update Changelog.md
calebmer b6ff87f
Update test case to detect option change after recycle
f653cd3
Restore ObserableQuery options when reusing after recycled
b452ebe
Set options to undefined instead of overriding directly
aa6795f
Fix typo
c9d20c0
Merge branch 'master' into master
jacobk 897f0e4
Merge branch 'master' into master
jacobk 965a395
Merge branch 'master' into master
helfer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ApolloClient, { ApolloError, ObservableQuery } from 'apollo-client'; | ||
import { NetworkInterface } from 'apollo-client/transport/networkInterface'; | ||
import { connect } from 'react-redux'; | ||
import { withState } from 'recompose'; | ||
|
||
|
@@ -2157,7 +2157,9 @@ describe('queries', () => { | |
); | ||
|
||
expect(Object.keys((client as any).queryManager.observableQueries)).toEqual(['1']); | ||
const queryObservable1 = (client as any).queryManager.observableQueries['1'].observableQuery; | ||
const queryObservable1: ObservableQuery<any> = (client as any).queryManager.observableQueries['1'].observableQuery; | ||
|
||
const originalOptions = Object.assign({}, queryObservable1.options); | ||
|
||
wrapper1.unmount(); | ||
|
||
|
@@ -2170,15 +2172,88 @@ describe('queries', () => { | |
); | ||
|
||
expect(Object.keys((client as any).queryManager.observableQueries)).toEqual(['1']); | ||
const queryObservable2 = (client as any).queryManager.observableQueries['1'].observableQuery; | ||
const queryObservable2: ObservableQuery<any> = (client as any).queryManager.observableQueries['1'].observableQuery; | ||
|
||
const recycledOptions = queryObservable2.options; | ||
|
||
expect(queryObservable1).toBe(queryObservable2); | ||
expect(recycledOptions).toEqual(originalOptions); | ||
|
||
wrapper2.unmount(); | ||
|
||
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']); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}); | ||
|
||
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' } ] } }; | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 ascache-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?There was a problem hiding this comment.
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:
Where
'xyz'
is the defaultfetchPolicy
. This should guarantee that thecache-only
policy is reset.There was a problem hiding this comment.
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 forfetchPolicy
and the currentsetOptions
implementation doesn't allow removing previously set properties:Ideally I'd like to remove both the
pollInterval
andfetchPolicy
completely before applyingoptions
insidereuse
.Any ideas? I'll commit the new breaking test case for now (which is testing for strict equality of options before and after recycle).
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 inreuse()
. 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 receivingupdateQueries
andrefetchQueries
instead of writing workarounds in the recycler…There was a problem hiding this comment.
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 toundefined
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 callingsetOptions
(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 ofapollo-client
'ssetOptions
are my only concern.There was a problem hiding this comment.
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 😣