Skip to content
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

Do not fire a query when changing options if unsubscribed #1154

Merged
merged 4 commits into from
Jan 20, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 29 additions & 7 deletions src/core/ObservableQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,8 @@ export class ObservableQuery<T> extends Observable<ApolloQueryResult<T>> {
};
}

// Note: if the query is not active (there are no subscribers), the promise
// will return null immediately.
public setOptions(opts: ModifiableWatchQueryOptions): Promise<ApolloQueryResult<T>> {
const oldOptions = this.options;
this.options = {
Expand All @@ -289,12 +291,10 @@ export class ObservableQuery<T> extends Observable<ApolloQueryResult<T>> {
}

// If forceFetch went from false to true or noFetch went from true to false
if ((!oldOptions.forceFetch && opts.forceFetch) || (oldOptions.noFetch && !opts.noFetch)) {
return this.queryManager.fetchQuery(this.queryId, this.options)
.then(result => this.queryManager.transformResult(result));
}
const tryFetch: boolean = (!oldOptions.forceFetch && opts.forceFetch)
|| (oldOptions.noFetch && !opts.noFetch);

return this.setVariables(this.options.variables);
return this.setVariables(this.options.variables, tryFetch);
}

/**
Expand All @@ -304,19 +304,41 @@ export class ObservableQuery<T> extends Observable<ApolloQueryResult<T>> {
* Note: if the variables have not changed, the promise will return the old
* results immediately, and the `next` callback will *not* fire.
*
* Note: if the query is not active (there are no subscribers), the promise
* will return null immediately.
*
* @param variables: The new set of variables. If there are missing variables,
* the previous values of those variables will be used.
*
* @param tryFetch: Try and fetch new results even if the variables haven't
* changed (we may still just hit the store, but if there's nothing in there
* this will refetch)
*/
public setVariables(variables: any): Promise<ApolloQueryResult<T>> {
public setVariables(variables: any, tryFetch: boolean = false): Promise<ApolloQueryResult<T>> {
const newVariables = {
...this.variables,
...variables,
};

if (isEqual(newVariables, this.variables)) {
const nullPromise = new Promise((resolve) => resolve(null));

if (isEqual(newVariables, this.variables) && !tryFetch) {
// If we have no observers, then we don't actually want to make a network
// request. As soon as someone observes the query, the request will kick
// off. For now, we just store any changes. (See #1077)
if (this.observers.length === 0) {
return nullPromise;
}

return this.result();
} else {
this.variables = newVariables;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@helfer I don't know why we don't set this.options.variables too here, but it seems weird. Why do we even have this.variables? I don't recall a good reason. Do you know?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, and I think there are / were some bugs related to that. We should just refactor to remove this.variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll do it in this PR then?


// See comment above
if (this.observers.length === 0) {
return nullPromise;
}

// Use the same options as before, but with new variables
return this.queryManager.fetchQuery(this.queryId, {
...this.options,
Expand Down
8 changes: 8 additions & 0 deletions test/ObservableQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,14 @@ describe('ObservableQuery', () => {
});
});

it('does not perform a query when unsubscribed if variables change', () => {
// Note: no responses, will throw if a query is made
const queryManager = mockQueryManager();
const observable = queryManager.watchQuery({ query, variables });

return observable.setVariables(differentVariables);
});

it('sets networkStatus to `setVariables` when fetching', (done) => {
const mockedResponses = [{
request: { query, variables },
Expand Down