-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Require Cache.EvictOptions when calling cache.evict. #6364
Conversation
We've been finding lately that named options APIs are much easier to work with (and later to extend). Since we're approaching a major new version of Apollo Client (3.0), and the cache.evict method did nothing in AC2 anyway, there's no sense preserving the alternate API with positional arguments.
// For backwards compatibility, evict can also take positional | ||
// arguments. Please prefer the Cache.EvictOptions style (above). | ||
public abstract evict( | ||
id: string, | ||
field?: string, | ||
args?: Record<string, any>, | ||
): boolean; |
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.
This comment was a bit of a tell, in hindsight.
public evict(options: Cache.EvictOptions): boolean { | ||
if (!options.id) { | ||
if (hasOwn.call(options, "id")) { | ||
// See comment in modify method about why we return false when | ||
// options.id exists but is falsy/undefined. | ||
return false; | ||
} | ||
options = { ...options, id: "ROOT_QUERY" }; | ||
} |
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.
This is important now that options.id
can be omitted as a way to default to the ROOT_QUERY
ID.
Here's the equivalent logic from cache.modify
:
public modify(options: ModifyOptions): boolean {
if (hasOwn.call(options, "id") && !options.id) {
// To my knowledge, TypeScript does not currently provide a way to
// enforce that an optional property?:type must *not* be undefined
// when present. That ability would be useful here, because we want
// options.id to default to ROOT_QUERY only when no options.id was
// provided. If the caller attempts to pass options.id with a
// falsy/undefined value (perhaps because cache.identify failed), we
// should not assume the goal was to modify the ROOT_QUERY object.
// We could throw, but it seems natural to return false to indicate
// that nothing was modified.
return false;
}
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.
Awesome @benjamn 👍!
We've been finding lately that named options APIs are much easier to work with (and later to extend), because you can think about the different options independently, and future additions of new named options tend to be much less disruptive for existing code.
Since we're approaching a major new version of Apollo Client (3.0), and the
cache.evict
method did nothing in AC2 anyway, there's no sense preserving the alternate API with positional arguments, now that we supportCache.EvictOptions
.In other words, this is definitely a breaking change, but only for those who have been using using the
@apollo/client
betas. Thank you for your patience and understanding; we know changes like this can be annoying if you were usingcache.evict
heavily, but we think the uniformity of always requiringCache.EvictOptions
will be simpler and more flexible in the long run.Here's a cheatsheet for updating your code: