Preserve no-cache fetchPolicy with notifyOnNetworkStatusChange #7761
Conversation
jcreighton
force-pushed
the
7611-preserve-no-cache-fetchpolicy
branch
from
680b796
to
bf08d7f
Compare
There was a problem hiding this comment.
This is on the right track, but I think this will make notifyOnNetworkStatusChange: true have no effect when fetchPolicy: "no-cache", which is probably safe/fine in many cases, but potentially not what people expect when they combine those options.
There may be an opportunity here to get rid of the cache-and-network trick altogether, since we have access to options.notifyOnNetworkStatusChange, options.fetchPolicy, and networkStatus inside the switch statement in fetchQueryByPolicy. I believe the notifyOnNetworkStatusChange trickery was only applicable for network-only and no-cache before (since all the other policies deliver an immediate result), so those might be the only two cases that need modification. Specifically, the network-only case would need to continue behaving like cache-and-network when notifyOnNetworkStatusChange is enabled, but the no-cache case would need to deliver an extra loading: true result without reading from the cache.
If this works out, the logic in these important methods will be a lot easier to reason about, which is exciting (to me, at least)!
jcreighton
force-pushed
the
7611-preserve-no-cache-fetchpolicy
branch
2 times, most recently
from
7d3f4f0
to
f5e0995
Compare
jcreighton
force-pushed
the
7611-preserve-no-cache-fetchpolicy
branch
from
f5e0995
to
8152b31
Compare
| const mightUseNetwork = | ||
| fetchPolicy === "cache-first" || | ||
| fetchPolicy === "cache-and-network" || | ||
| fetchPolicy === "network-only" || | ||
| fetchPolicy === "no-cache"; | ||
|
|
||
| if (mightUseNetwork && | ||
| notifyOnNetworkStatusChange && | ||
| typeof oldNetworkStatus === "number" && | ||
| oldNetworkStatus !== networkStatus && | ||
| isNetworkRequestInFlight(networkStatus)) { | ||
| // In order to force delivery of an incomplete cache result with | ||
| // loading:true, we tweak the fetchPolicy to include the cache, and | ||
| // pretend that returnPartialData was enabled. | ||
| if (fetchPolicy !== "cache-first") { | ||
| fetchPolicy = "cache-and-network"; | ||
| } | ||
| returnPartialData = true; | ||
| } |
jcreighton
force-pushed
the
7611-preserve-no-cache-fetchpolicy
branch
from
1d79890
to
0ba32b3
Compare
jcreighton
force-pushed
the
7611-preserve-no-cache-fetchpolicy
branch
from
ac1348a
to
b4cd8fd
Compare
…cache-fetchpolicy" This reverts commit 319993e, reversing changes made to 6120401. Because #7611 could be a visible change for some applications, we've decided to relocate these changes onto the release-3.4 branch, rather than main, so we revert the PR on main, merge main into release-3.4, then un-revert the PR on the release-3.4 branch.
Shouldn't this be controlled by the |
I agree with @kliakos. This created an issue in our large codebase because the types are not correct anymore. TypeScript expects a non-nullable field, but receives |
Fixes #7611 & #6998. While we want to force delivery for an incomplete cache result with
loading: true, we should preserve theno-cachefetchPolicy.An additional check has been added to avoid resetting the fetch policy to "cache-and-network" when it's set to "no-cache".Thanks to @benjamn's feedback below, the "notifyOnNetworkStatusChangechanges the fetch policy" trickery has been removed. The logic behind that is still valid but it's now centralized infetchQueryByPolicy'sswitchstatement. This means fetch policies are now preserved even withnotifyOnNetworkStatusChangeset.Checklist: