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

Allow passing function to useQuery for finer-grained options control #9223

Merged
merged 5 commits into from
Mar 10, 2022

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Dec 20, 2021

A pretty serious limitation of the useQuery(query, options) API is that it encourages passing a fresh bag of options as the second parameter every time the component (re)renders, potentially overwriting previous options from previous renders, with no way to tell which options should be taken merely as default values and which should override existing options. This ambiguity becomes especially problematic when options like options.fetchPolicy are updated elsewhere, but that work is undone the next time useQuery(query, { fetchPolicy: <original policy> }) is called:

useQuery(QUERY, {
  // Was this just a default value, or should it become the new fetch policy?
  // With the current useQuery API, there's no safe way to guess!
  fetchPolicy: "cache-and-network",
})

A more minor problem with the useQuery options API is that it's somewhat wasteful (in time and memory) to recreate a new options object on every render, even if it will be safely ignored/discarded. The garbage still has to be collected, so it would be convenient (when it matters) to have a way to avoid recreating options unnecessarily.

This PR introduces the option of passing a function as the second parameter to useQuery, which will be immediately invoked with the existing/current options (an empty object on first render), and should return new options (possibly including some unchanged existing options) to be merged with the existing options.

Crucially, this functional style allows taking existing options into account when determining the new options to use, which means it's now possible to (re)use existing options as default values:

useQuery(QUERY, ({
  fetchPolicy = "cache-and-network",
  notifyOnNetworkStatusChange = true,
}) => ({
  // This option defaults to "cache-and-network" if not previously defined.
  fetchPolicy,

  // This option defaults to true if not previously defined.
  notifyOnNetworkStatusChange,

  // It's probably a good idea to pass fresh onCompleted and/or onError functions,
  // since the callbacks from previous options may have the wrong closure/scope.
  onCompleted() {...},
}))

This function defaults to the current options.fetchPolicy and options.notifyOnNetworkStatusChange to avoid overriding them on subsequent renders, but overrides onCompleted unconditionally.

Note that you do not need to worry about capturing/preserving ...rest properties, as that happens automatically, thanks to merging the returned options with the existing options (mentioned above):

useQuery(QUERY, ({
  fetchPolicy = "cache-and-network",
  ...otherOptions, // Not necessary!
}) => ({
  fetchPolicy,

  // Not necessary, because returned options will be merged with existing options,
  // so existing options remain unchanged by default.
  ...otherOptions,
}))

However, if you really want to take control of things, you can skip this automatic merging by modifying the existing options object and returning it === as given:

useQuery(QUERY, existingOptions => {
  if (typeof existingOptions.fetchPolicy === "undefined") {
    existingOptions.fetchPolicy = "network-only";
  }
  // Perform any other modifications to existingOptions before returning it,
  // including deleting properties...
  return existingOptions;
})

If you only need to provide default values (a common use case) rather than forcing any options (like onCompleted), you could imagine implementing a defaultOptions helper function that makes this a bit more ergonomic:

useQuery(QUERY, defaultOptions({
  fetchPolicy: "cache-and-network",
  notifyOnNetworkStatusChange: true,
}))

I'll leave that implementation as an exercise for the reader, since this description is already running a bit long.

This PR is still a draft because I need to write tests and documentation, but I wanted to get it out there for discussion sooner rather than later.

@benjamn benjamn added this to the Release 3.6 milestone Dec 20, 2021
@benjamn benjamn self-assigned this Dec 20, 2021
@benjamn benjamn changed the title Allow passing _function_ to useQuery for finer-grained options control Allow passing function to useQuery for finer-grained options control Dec 20, 2021
@hwillson hwillson added 2022-01 and removed 2021-12 labels Jan 3, 2022
@benjamn benjamn force-pushed the allow-passing-options-function-to-useQuery branch from 89daec3 to f22c54c Compare February 4, 2022 20:43
@benjamn benjamn changed the base branch from main to release-3.6 February 4, 2022 20:44
@benjamn benjamn marked this pull request as ready for review February 4, 2022 20:44
@hwillson hwillson mentioned this pull request Feb 15, 2022
7 tasks
@benjamn benjamn force-pushed the allow-passing-options-function-to-useQuery branch from f22c54c to 803d110 Compare March 2, 2022 22:50
@benjamn benjamn changed the base branch from release-3.6 to useQuery-internal-state-ref March 2, 2022 22:51
@benjamn benjamn force-pushed the allow-passing-options-function-to-useQuery branch from 803d110 to 91d69e6 Compare March 2, 2022 22:53
benjamn added a commit that referenced this pull request Mar 2, 2022
package.json Outdated
"maxSize": "28.6kB"
"maxSize": "28.95kB"
Copy link
Member Author

Choose a reason for hiding this comment

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

Most of this increase comes from #9459, where I have not updated the npm run bundlesize limit yet. The actual net increase from this PR is +0.05kB.

@benjamn
Copy link
Member Author

benjamn commented Mar 2, 2022

@hwillson @brainkim I've rebased this PR against the branch for #9459, so we're ready to merge this PR into release-3.6 soon after we merge that PR. I also added a handful of basic tests, so this PR is easier to review and safer to merge.

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.

Thanks @benjamn, looks great! We'll need docs for this but we can add those after this is merged. Honestly your great PR description could probably just be dropped in the docs as is!

@benjamn benjamn force-pushed the allow-passing-options-function-to-useQuery branch from a0ce008 to 1d5218f Compare March 10, 2022 17:29
@benjamn benjamn changed the base branch from useQuery-internal-state-ref to release-3.6 March 10, 2022 17:30
@benjamn benjamn merged commit 7df3d4e into release-3.6 Mar 10, 2022
@benjamn benjamn deleted the allow-passing-options-function-to-useQuery branch March 10, 2022 17:36
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 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