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

Add a new useLoadableQuery hook #11300

Merged
merged 201 commits into from Nov 28, 2023
Merged

Conversation

jerelmiller
Copy link
Member

@jerelmiller jerelmiller commented Oct 17, 2023

Closes #11349

Introduces a new useLoadableQuery hook. This hook works similarly to useBackgroundQuery in that it returns a queryRef that can be used to suspend a query via useReadQuery. Its goal is to provide a more ergonomic way to load the query during a user interaction (for example when wanting to preload some data) that would otherwise be clunky with useBackgroundQuery.

function App() {
  // `queryRef` is `null` until `loadQuery` is called the first time
  const [loadQuery, queryRef, { refetch, fetchMore, reset }] = useLoadableQuery(query, options)

  return (
    <>
      <button onClick={() => loadQuery(variables)}>Load query</button>
      <button onClick={() => reset()}>Reset query</button>
      <Suspense fallback={<SuspenseFallback />}>
        {queryRef && <Child queryRef={queryRef} />}
      </Suspense>
    </>
  );
}

function Child({ queryRef }) {
  const { data } = useReadQuery(queryRef)

  // ...
}

@changeset-bot
Copy link

changeset-bot bot commented Oct 17, 2023

🦋 Changeset detected

Latest commit: c20e373

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 17, 2023

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 37.7 KB (+1.16% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 44.16 KB (+1.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 42.64 KB (+1.06% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 32.78 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 31.43 KB (+0.01% 🔺)
import { ApolloProvider } from "dist/react/index.js" 1.28 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production) 1.26 KB (0%)
import { useQuery } from "dist/react/index.js" 4.34 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.16 KB (-0.03% 🔽)
import { useLazyQuery } from "dist/react/index.js" 4.65 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.47 KB (-0.03% 🔽)
import { useMutation } from "dist/react/index.js" 2.61 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.59 KB (-0.04% 🔽)
import { useSubscription } from "dist/react/index.js" 2.29 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 2.25 KB (-0.05% 🔽)
import { useSuspenseQuery } from "dist/react/index.js" 4.33 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production) 3.79 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 3.82 KB (-0.03% 🔽)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.27 KB (0%)
import { useReadQuery } from "dist/react/index.js" 3.05 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production) 3 KB (-0.13% 🔽)
import { useFragment } from "dist/react/index.js" 2.15 KB (0%)
import { useFragment } from "dist/react/index.js" (production) 2.1 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" 4.09 KB (+100% 🔺)
import { useLoadableQuery } from "dist/react/index.js" (production) 3.53 KB (+100% 🔺)

@jerelmiller jerelmiller added this to the Release 3.9 milestone Nov 7, 2023
@jerelmiller

This comment was marked as resolved.

@jerelmiller jerelmiller removed this from the Release 3.9 milestone Nov 8, 2023
@jerelmiller jerelmiller changed the title [WIP] Add a new useInteractiveQuery hook Add a new useInteractiveQuery hook Nov 10, 2023
@jerelmiller jerelmiller linked an issue Nov 10, 2023 that may be closed by this pull request
@jerelmiller jerelmiller changed the title Add a new useInteractiveQuery hook Add a new useLoadableQuery hook Nov 10, 2023
@jerelmiller jerelmiller marked this pull request as ready for review November 10, 2023 19:51
@jerelmiller
Copy link
Member Author

/release:pr

Copy link
Contributor

A new release has been made for this PR. You can install it with npm i @apollo/client@0.0.0-pr-11300-20231110195441.

@jerelmiller
Copy link
Member Author

/release:pr

Copy link
Contributor

A new release has been made for this PR. You can install it with npm i @apollo/client@0.0.0-pr-11300-20231111004144.

@jerelmiller
Copy link
Member Author

I think this hook should also include a reset method that resets the queryRef back to null and disposes of the current query ref. Depending on code review timing, I may do this as a followup PR.

Copy link
Member

@phryneas phryneas left a comment

Choose a reason for hiding this comment

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

This looks very good!

I've left some small nitpicks/ideas, but nothing major :)

src/testing/matchers/index.d.ts Outdated Show resolved Hide resolved
src/testing/matchers/ProfiledComponent.ts Outdated Show resolved Hide resolved
src/react/types/types.ts Show resolved Hide resolved
src/react/hooks/useLoadableQuery.ts Outdated Show resolved Hide resolved
data: { hello: "from cache" },
networkStatus: NetworkStatus.ready,
error: undefined,
});
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add something like expect(ReadQueryHook).not.toRerender() to make sure nothing else will come in after?

expect(SuspenseFallback).not.toHaveRendered();
});

it('suspends and does not use partial data when changing variables and using a "cache-first" fetch policy with returnPartialData', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('suspends and does not use partial data when changing variables and using a "cache-first" fetch policy with returnPartialData', async () => {
it('suspends and does not use empty partial data when changing variables and using a "cache-first" fetch policy with returnPartialData', async () => {

this confused me a bit at first, but you mean it won't use a non-existing/empty entry as "partial data", right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya this could use some clarifying behavior here. The idea with this test is that when returnPartialData is true and I have partial data for 1 set of variables, it didn't try and reuse that partial data for a different set of variables. It should start with a clean slate and suspend to fetch the entire data set.

Copy link
Member

Choose a reason for hiding this comment

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

Any way you can get that in there? It makes sense now, but didn't get too obvious from the description, or the test in combination with the description. Could also just be a long comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! Updated in b84f0d5. Hope this helps clear things up :)

[queryRef]
);

const loadQuery: LoadQuery<TVariables> = React.useCallback(
Copy link
Member

Choose a reason for hiding this comment

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

Crazy thought:

Could we maybe prevent this function from being executed before the first render committed by throwing an exception?

It might prevent some abuse scenarios where other hooks might be better suited 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that idea. I believe Relay does something similar with its useQueryLoader hook where it prevents you from calling its function during any render, so its definitely a pattern out there in the wild today.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in 9a6c748. Fair warning, it might be a tad controversial. BUT Relay has something similar, so I felt it was ok.

@jerelmiller
Copy link
Member Author

@phryneas this should be fully updated with the updated profiler API and is ready for another look!

@jerelmiller
Copy link
Member Author

/release:pr

Copy link
Contributor

A new release has been made for this PR. You can install it with npm i @apollo/client@0.0.0-pr-11300-20231128055743.

Copy link
Member

@phryneas phryneas left a comment

Choose a reason for hiding this comment

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

I'm through all the files, but only halfway through the tests.
I'm already submitting my accumulated comments since we have meetings coming up now.

LoadQueryFunction<TVariables>,
QueryReference<TData> | null,
{
fetchMore: FetchMoreFunction<TData, TVariables>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fetchMore: FetchMoreFunction<TData, TVariables>;
/** {@inheritDoc @apollo/client!ObservableQuery#fetchMore:member(1)} */
fetchMore: FetchMoreFunction<TData, TVariables>;

fetchMore actually doesn't have a comment in ObservableQuery yet, so this might be a good time to add one? ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to hold off on this update for a followup PR. I'd like to hit useSuspenseQuery and useBackgroundQuery as well with this change. This allows me to keep this PR focused on useLoadableQuery as much as possible.

QueryReference<TData> | null,
{
fetchMore: FetchMoreFunction<TData, TVariables>;
refetch: RefetchFunction<TData, TVariables>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
refetch: RefetchFunction<TData, TVariables>;
/** {@inheritDoc @apollo/client!ObservableQuery#refetch:member(1)} */
refetch: RefetchFunction<TData, TVariables>;

The refetch comment of ObservableQuery doesn't read perfect for the hooks, but it might at least be a starting point until we have some central interface with all the hook options to pick from
(it reads:)

/**

  • Update the variables of this observable query, and fetch the new results.
  • This method should be preferred over setVariables in most use cases.
  • @param variables: The new set of variables. If there are missing variables,
  • the previous values of those variables will be used.
    */

Copy link
Member Author

Choose a reason for hiding this comment

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

Love it! Will get this in a followup PR to ensure useBackgroundQuery and useSuspenseQuery can benefit as well.

src/react/hooks/internal/useRenderGuard.ts Show resolved Hide resolved
src/react/hooks/useLoadableQuery.ts Show resolved Hide resolved
src/react/hooks/useLoadableQuery.ts Show resolved Hide resolved
src/react/hooks/__tests__/useLoadableQuery.test.tsx Outdated Show resolved Hide resolved
Comment on lines +456 to +459
// Resetting the result allows us to detect when ReadQueryHook is unmounted
// since it won't render and overwrite the `null`
Profiler.mergeSnapshot({ result: null });

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Resetting the result allows us to detect when ReadQueryHook is unmounted
// since it won't render and overwrite the `null`
Profiler.mergeSnapshot({ result: null });
Profiler.mergeSnapshot({ queryRef });

What do you think about tracking the queryRef here instead?

Also, this makes me think... Maybe we should also track unmountedComponents or internally track which components are currently mounted and which are not, so we can make assumptions on that?
This could happen internally in useTrackRender

const { tree } = await Profiler.takeRender();
expect(Component).toBeMountedIn(tree)
expect(Component).not.toBeMountedIn(tree)
expect("ComponentName").toBeMountedIn(tree)
expect("ComponentName").not.toBeMountedIn(tree)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking the same. I hit this case at the end of the day yesterday and figured I'd revisit today. It's not obvious when something has unmounted so something like this would be much nicer. I love the matcher you have here as well so I'm borrowing that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to do this in a separate PR so we can provide some good discussion if need be and move this PR forward.

src/react/hooks/__tests__/useLoadableQuery.test.tsx Outdated Show resolved Hide resolved
src/react/hooks/__tests__/useLoadableQuery.test.tsx Outdated Show resolved Hide resolved
src/react/hooks/__tests__/useLoadableQuery.test.tsx Outdated Show resolved Hide resolved
@jerelmiller jerelmiller merged commit a815873 into release-3.9 Nov 28, 2023
30 checks passed
@jerelmiller jerelmiller deleted the jerel/use-interactive-query branch November 28, 2023 23:44
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build a new useLoadableQuery hook
3 participants