-
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
Replace experimental useQuery
options functions with simpler options.defaultOptions
option
#9563
Replace experimental useQuery
options functions with simpler options.defaultOptions
option
#9563
Conversation
// Default WatchQueryOptions for this useQuery, providing initial values for | ||
// unspecified options, superseding client.defaultOptions.watchQuery (option | ||
// by option, not whole), but never overriding options previously passed to | ||
// useQuery (or options added/modified later by other means). | ||
// TODO What about about default values that are expensive to evaluate? | ||
defaultOptions?: Partial<WatchQueryOptions<TVariables, TData>>; |
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.
Here's where the new defaultOptions
field is defined, with an explanatory comment.
The TODO
is still valid, though maybe defaultOptions
could define lazy getter properties somehow? Not a blocker for this PR.
This looks good! I haven’t looked too closely at the new tests but I ran out of time. On to 3.6! |
@@ -70,8 +70,8 @@ type OptionsUnion<TData, TVariables, TContext> = | |||
export function mergeOptions< | |||
TOptions extends OptionsUnion<any, any, any> | |||
>( | |||
defaults: Partial<TOptions>, | |||
options: TOptions, | |||
defaults: TOptions | Partial<TOptions>, |
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.
I’m pretty sure TOptions
and Partial<TOptions>
are the same thing in this position but I have been mistaken.
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.
Using the union TOptions | Partial<TOptions>
in both positions helped in some cases where only one of the input argument types was a complete WatchQueryOptions
, which I still wanted to produce a complete WatchQueryOptions
instead of Partial<WatchQueryOptions>
for the mergeOptions
result type. 🤷
After living/playing with the changes introduced in #9223 for the past few weeks, I've become convinced the options function idea was overcomplicated for the functionality it provided, and the majority of use cases for options functions (providing default/initial option values) can be served by much simpler API changes.
Specifically, this PR adds a new
defaultOptions
field to the existingQueryHookOptions
interface (the type of theuseQuery
options parameter), which allows specifying certain options (anyWatchQueryOption
fields) as merely default/initial values, giving precedence to any existing options of the same name found inobservableQuery.options
. Globalclient.defaultOptions.watchQuery
options are also included, at lower precedence than thedefaultOptions
passed directly touseQuery
.If you prefer code, this experimental style is what we're replacing:
And this is what it becomes:
A significant advantage of the
defaultOptions
approach over options functions is that you can still provide mandatory, non-default options (likeonCompleted
andonError
in the example above) at the same time as providingdefaultOptions
, which gives the developer full expressive control over this important distinction.In my opinion (even as their designer), the syntax of options functions was also pretty clunky, which would be acceptable for an obscure feature designed to stay hidden until you need it, but not so much for a style we want to recommend as the preferred way of passing (most?) options, especially options that can meaningfully change over time, like
fetchPolicy
.Finally, since the
useQuery
options function would only ever see theQueryHookOptions
(never theWatchQueryOptions
that really count), changes toobservableQuery.options
would be invisible to options functions, which defeats one of the main goals of PR #9223 (the ability to give precedence to current options). This objection goes beyond stylistic criticism, which is why it seemed important not just to introducedefaultOptions
but to remove the function-based options-passing style fromuseQuery
anduseLazyQuery
, effectively reverting #9223.