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

Implement useBackgroundQuery hook for use with useFragment #8783

Closed
wants to merge 8 commits into from

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Sep 13, 2021

Building on #8782, this new useBackgroundQuery hook allows a component to fetch data in a way that's managed by the React component lifecycle, without automatically subscribing to every future change in that data, allowing child components to update in place without requiring the parent component to rerender all its children.

In other words, useBackgroundQuery is my best answer so far to this question I raised in #8236 (comment):

[…] there are some remaining questions to answer around who (what code) is responsible for issuing the queries, in this new world of only/mostly fragments.

Reusing useQuery is certainly an option, but then the component gets subscribed to all changes in the query data, when it probably only cares about some small fragment of the query's data. I think a new kind of query hook will be necessary, or perhaps an additional sub-section within the useFragment options.

This API is still in development and is very much subject to change (hence the [WIP] tag). Watch this space for more details and examples in the coming days/week.

@@ -0,0 +1,143 @@
import { useRef, useEffect } from "react";
import { unstable_batchedUpdates } from "react-dom";
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we can’t use useBackgroundQuery in React Native?

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently react-native also exports unstable_batchedUpdates, so maybe we can find a way to swap the source module when running in React Native?

@benjamn benjamn force-pushed the useFragment-hook branch 4 times, most recently from 1e20e8f to b2a2a1d Compare February 4, 2022 17:51
@benjamn benjamn changed the title [WIP] Implement useBackgroundQuery hook Implement useBackgroundQuery hook for use with useFragment Feb 4, 2022
Copy link
Contributor

@brainkim brainkim left a comment

Choose a reason for hiding this comment

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

Interesting work! Some thoughts on the proposed API.

  • I’m not sure about the use-case of passing an ObservableQuery directly into useBackgroundQuery(). As always, I dislike this sort of polymorphism.
  • I’m still not clear on the motivation behind using a “higher-order hook” even if it doesn’t break “The Rules of Hooks.” I get that the point of useBackgroundQuery() is so we can have loading, networkStatus, error and data each listened to and cause re-renders individually, but these properties all seem to change in near perfect unison, especially when you use options like notifyOnNetworkStatusChange. I also dislike piping this all through reactive vars because I imagine this will be hard to debug.
  • I am very worried that this hook’s primary purpose is to coordinate with the useFragment() hook, and yet there doesn’t seem to be any sort of means for ensuring that each useFragment() call is backed by a corresponding useBackgroundQuery() call. I imagine a lot of developers would spend time carefully making sure each fragment was used in a useBackgroundQuery() call. What is the guidance for how these two hooks should be called? Should the useBackgroundQuery() hook be called in a parent component and useFragment() be called in a child component? Does it make sense to call the two hooks from the same component? I wonder if we could be using React contexts for this API.

useError,
} = useBackgroundQuery({
query,
notifyOnNetworkStatusChange: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the usage of notifyOnNetworkStatusChange intentional here? It feels like an anti-pattern given the intent of the hook.

},

error(error) {
unstable_batchedUpdates(() => {
Copy link
Contributor

@brainkim brainkim Feb 16, 2022

Choose a reason for hiding this comment

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

Do we not have to do the clearing of results stuff during errors that we do in the useQuery() hook?

ref.current = isObservableQuery(queryOrOptions)
? internalStateFromObservableQuery(queryOrOptions)
: internalStateFromOptions(client, queryOrOptions)
);
Copy link
Contributor

@brainkim brainkim Feb 16, 2022

Choose a reason for hiding this comment

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

Nit: all three of these functions could likely be inlined insofar as they’re all only referenced here.

) {
observable.setOptions(mergeOptions(
observable.options,
state.options = queryOrOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a sneaky place to update a ref property! It took me a while that we’re assigning it from here.


useEffect(() => {
if (
!isObservableQuery(queryOrOptions) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we thought about the edge case of going from options to ObservableQuery or from ObservableQuery to options?

…Query.

This design lets the component selectively depend on ObservableQuery
metadata that it cares about, rather than always rerendering the
component whenever any item of that metadata has been updated.

For example, the useLoading hook uses useReactiveVar to read from
result.loadingVar, which is kept in sync with the result.loading
properties of any results received by the ObservableQuery. Likewise for
useNetworkStatus, useError, and useData, though useData is especially
discouraged because it defeats the main purpose of useObservableQuery,
which is to avoid rerendering the component tree every time anything
triggers a change in result.data.

The ObservableQuery itself is now returned as the result.observable
property from useObservableQuery, which gives us room to add additional
sibling properties in the future.
* Return most recent result diff from useFragment, add test

* fix: move test to useFragment.test.tsx

* fix: incorporate PR feedback on useFragment test

* fix: avoid calling diffToResult twice
@jpvajda jpvajda modified the milestones: Release 3.7, Release 3.8 Jul 26, 2022
@alessbell
Copy link
Member

useBackgroundQuery appears to be headed in a different direction (see #8694 (comment)).

For that reason, I'm going to close this PR (but keep the branch around). Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: useBackgroundQuery hook
6 participants