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

Leave fetchPolicy unchanged when skip: true (or in standby) and nextFetchPolicy is available #9823

Merged
merged 7 commits into from Jun 19, 2022

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jun 14, 2022

Fixes issue #9765, judging by the reproduction provided by @Titozzz.

It may be worth noting that skip: true and fetchPolicy: "standby" are intended to be essentially synonymous, which is why I sometimes mention both together (like "standby/skip:true").

@benjamn benjamn added this to the v3.6.x patch releases milestone Jun 14, 2022
@benjamn benjamn self-assigned this Jun 14, 2022
Comment on lines -1159 to +1176
concast.cleanup(() => {
this.fetchCancelFns.delete(queryId);

if (queryInfo.observableQuery) {
queryInfo.observableQuery["applyNextFetchPolicy"]("after-fetch", options);
}
});
concast.promise.then(cleanupCancelFn, cleanupCancelFn);
Copy link
Member Author

Choose a reason for hiding this comment

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

In PR #9718, which has only landed on the release-3.7 branch so far, we replaced concast.cleanup with an improved (internal) API called concast.beforeNext. However, if we can just stop using concast.cleanup, that would be another solution to the problems it has caused. There's one more spot where we rely on it, in getObservableFromLink.

concastSources.length > 0 &&
queryInfo.observableQuery
) {
queryInfo.observableQuery["applyNextFetchPolicy"]("after-fetch", options);
Copy link
Member Author

Choose a reason for hiding this comment

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

The applyNextFetchPolicy method is now called synchronously, immediately after the fetch policy was used to make a request (by calling fetchQueryByPolicy above). This required some test changes where we previously assumed the fetchPolicy should still be the same immediately after performing some async operation like refetching, but I strengthened several of those tests to show that the original fetchPolicy is what's used when fetching. I consider this change an improvement since it eliminates the possibility of more than one query using the original fetchPolicy before nextFetchPolicy is applied.

Comment on lines +656 to +657
if (fetchPolicy === "standby") {
// Do nothing, leaving options.fetchPolicy unchanged.
Copy link
Member Author

Choose a reason for hiding this comment

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

Not only do we avoid calling applyNextFetchPolicy when options.fetchPolicy === "standby", but this empty conditional block ensures applyNextFetchPolicy has no effect in that case. Defense in depth!

@benjamn benjamn force-pushed the issue-9765-leave-fetchPolicy-unchanged-when-skip-true branch from 698e155 to 21b4f56 Compare June 14, 2022 19:39
@benjamn benjamn linked an issue Jun 14, 2022 that may be closed by this pull request
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Awesome - thanks @benjamn!

@benjamn benjamn merged commit a3aefcb into main Jun 19, 2022
@benjamn benjamn deleted the issue-9765-leave-fetchPolicy-unchanged-when-skip-true branch June 19, 2022 22:02
benjamn added a commit to dylanwulf/apollo-client that referenced this pull request Jun 27, 2022
benjamn added a commit to dylanwulf/apollo-client that referenced this pull request Jun 27, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skip parameter not working properly Skip not skipping query in useQuery
2 participants