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

Clarify mutation fetchPolicy options using MutationFetchPolicy union type #8602

Merged
merged 2 commits into from
Aug 9, 2021

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Aug 6, 2021

These are the changes I was suggesting in my comments on PR #8595:

I believe this addition of MutationFetchPolicy is a backwards-compatible change, since it amounts to replacing a type that could only be "no-cache" | undefined with a strictly additive superset: "network-only" | "no-cache" | undefined.

I don't expect anyone to want/need to pass fetchPolicy: "network-only" explicitly, but I think it makes sense to be explicit about the default mutation fetchPolicy behavior, and its relationship to the "no-cache" alternative.

Comment on lines +33 to +37
export type MutationFetchPolicy = Extract<
FetchPolicy,
| 'network-only' // default behavior (mutation results written to cache)
| 'no-cache' // alternate behavior (results not written to cache)
>;
Copy link
Member Author

Choose a reason for hiding this comment

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

This type is ultimately just 'network-only' | 'no-cache', but "extracting" that union from FetchPolicy (which is also just a union of strings) felt like the right thing to do to convey the code's intent.

benjamn added 2 commits August 9, 2021 11:41
…type.

These are the changes I was suggesting in my comments on PR #8595:
#8595 (review)

I believe this is a backwards-compatible change, since it amounts to
replacing a type that could only be `"no-cache" | undefined` with a
strictly additive superset: `"network-only" | "no-cache" | undefined`.

I don't expect anyone to want/need to pass `fetchPolicy: "network-only"`
explicitly, but I think it makes sense to be explicit about the default
`fetchPolicy` and its relationship to the `"no-cache"` alternative.
@benjamn benjamn force-pushed the MutationFetchPolicy branch from 673af3b to 83eb9d5 Compare August 9, 2021 15:44
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.

Looks great @benjamn - thanks!

@benjamn benjamn merged commit ccd1cf7 into main Aug 9, 2021
@benjamn benjamn deleted the MutationFetchPolicy branch August 9, 2021 16:14
@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.

2 participants