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

feat(rtk-query/react): add useUnstable_SuspenseQuery hook #2149

Closed

Conversation

FaberVitale
Copy link
Contributor

@FaberVitale FaberVitale commented Mar 20, 2022

Description

Adds useUnstable_SuspenseQuery, an alternative to useQuery that can be used to trigger <Suspense /> while fetching data.

Accepts the same arguments of useQuery and adds the following options:

export interface UseSuspenseOptions {
  /**
   * If set to `true` it will suspend the query on subsequent fetches,
   * hence when `data !== undefined && isFetching` is truthy.
   */
  suspendOnRefetch?: boolean
}

Returns useQuery output without isLoading(useless) and status(deprecated).

Demo


Other render-as-you-fetch implementations

Todo

  • test with react 18
  • add more test, especially test the interaction with error boundaries
  • bike shedding 😎
  • clean up TS types

References


Please provide feedback and suggestions 🙏

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 20, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f2d0ec6:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration
@examples-query-react/suspense Configuration
@examples-action-listener/counter Configuration
reduxjs/redux-toolkit PR

@netlify
Copy link

netlify bot commented Mar 20, 2022

Deploy Preview for redux-starter-kit-docs ready!

Name Link
🔨 Latest commit f2d0ec6
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/623eddcebe30e600093eb552
😎 Deploy Preview https://deploy-preview-2149--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@FaberVitale FaberVitale changed the title feat(rtk-query/react): add useUnstable_SuspenseQuery & suspense example feat(rtk-query/react): add useUnstable_SuspenseQuery hook Mar 20, 2022
)

if (pendingPromise) {
throw pendingPromise
Copy link
Member

Choose a reason for hiding this comment

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

If I'm reading this correctly, this would work like this right now:

  • render the component once (loading true, no promise)
  • fire the effect to make a request
  • render the component again
  • throw into suspense

Could we also achieve that without the initial render? The initial render kind of destroys the promise of "data will always be available, you don't need to check for a isLoading flag".

Copy link
Member

Choose a reason for hiding this comment

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

(that said, I don't want to downplay the work you are putting into this - I'm very happy to see you picking this up!)

Copy link
Contributor Author

@FaberVitale FaberVitale Mar 21, 2022

Choose a reason for hiding this comment

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

If I'm reading this correctly, this would work like this right now:

* render the component once (loading true, no promise)

* fire the effect to make a request

* render the component again

* throw into suspense

Could we also achieve that without the initial render? The initial render kind of destroys the promise of "data will always be available, you don't need to check for a isLoading flag".

You're right; thank you for your feedback 👍

Triggering a prefetch solves this issue:

+            dispatch(api.util.prefetch(name as any, arg as any, { ifOlderThan: false }))
+
             const pendingPromise = api.util.getRunningOperationPromise<any>(
               name,
               arg as unknown as string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in a6b7f36

@markerikson
Copy link
Collaborator

markerikson commented Mar 21, 2022

An interesting comment from Reactiflux:

at this point I think const { data } = useSuspendedThing() is actually an antipattern. It breaks down when you need to query things in parallel

const { thing1 } = useSuspendedThing1();
const { thing2 } = useSuspendedThing2();

oopsy daisy waterfall. the way I think suspense is suppose to work is like

const { readThing1 } = useSuspendedThing1(); // start loading
const { readThing2 } = useSuspendedThing2();

const thing1 = readThing1(); // select data
const thing2= readThing2();

you suspend when you read not when you load
Relay works this way. useFragment selects and suspends while useLazyLoadQuery can just load

In other words, the hooks start the request immediately, and it's the read() calls that throw the promises and suspend.

I think I can sorta see what's being said here.

Something to consider.

@wuweiweiwu
Copy link
Contributor

wuweiweiwu commented Mar 21, 2022

I do think its worth clarifying that the reactiflux example with relay is akin to existing render-then-you-fetch patterns as you have the pass the data from useLazyLoadQuery into useFragment to access the data so suspense boundaries will never be triggered since the data field is only available once the fetch has finished and thus the useFragment call will never suspend. to use suspense for data fetching in relay you would have to use either useQuery or usePreloadedQuery

// render-then-you-fetch
// will never trigger suspense

// parent component
// data is null until the fetch is complete
const { data } = useLazyQuery(...)

data && <ChildComponent data={data} />

// child component
const { data } = useFragment(..., props.data)

it is true that in relay if you have subsequent useQuery calls that will cause a request waterfall while using suspense. the "idiomatic" way to avoid that is to preload the query via useQueryLoader (or EntryPoints) to preload the query in a parent component and thus all of the fetches are kicked off in parallel. it is then consumed in the child component via usePreloadedQuery It is quite annoying to have to write all the boilerplate for preloading to properly use suspense with data fetching without request waterfalls.

// render-as-you-fetch

// parent component
// doesn't trigger suspense
const [preloadedQuery] = useQueryLoader(...)
// can kick off fetches in parallel
// const [preloadedQuery2] = useQueryLoader(...)
// const [preloadedQuery3] = useQueryLoader(...)

<ChildComponent preloadedQuery={preloadedQuery} />

// child component
// this will trigger a suspense boundary until the query is finished
const { data } = usePreloadedQuery(..., props.preloadedQuery);

another interesting pattern i've seen is with recoil's waitForAll selector which is kinda saga-like also reminds me of Promise.all or Promise.allSettled. usage doesn't require preloading and doesn't cause request waterfalls. instead fetches are sent in parallel

const [data1, data2] = useRecoilValue(waitForAll([dataSource1, dataSource2]))

another interesting approach is this swr RFC which has an interesting api which involves wrapping hook calls in another hook (which break the rules of hooks) or using hook calls to delineate suspense boundaries

at this point I think const { data } = useSuspendedThing() is actually an antipattern. It breaks down when you need to query things in parallel

i would argue that this is the pattern for using data fetching with suspense. both preloading or some sort of Promise.all for queries would solve for the parallel use case (as seen in relay and recoil). if you want to achieve render-as-you-fetch you'd always have to decouple initiating the data fetch from reading results (e.g prefetching)

getRandomIntervalValue()
)

const { data, isFetching, refetch } =
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way to type isFetching as void or remove it from the return value if suspendOnRefetch is true?

if suspendOnRefetch is true, isFetching is always going to be false when accessed in user code

Copy link
Contributor Author

@FaberVitale FaberVitale Mar 26, 2022

Choose a reason for hiding this comment

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

is there a way to type isFetching as void or remove it from the return value if suspendOnRefetch is true?

if suspendOnRefetch is true, isFetching is always going to be false when accessed in user code

You can play with ts types in order to have isFetching be always true if suspendOnRefetch is set to true but,

no I don't think that we should remove isFetching from the response.

}
} else if (
queryStateResults.isError &&
!queryStateResults.isFetching
Copy link
Contributor

@wuweiweiwu wuweiweiwu Mar 21, 2022

Choose a reason for hiding this comment

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

I think this is interesting behavior. Not sure how everyone would expect this to behave.

i would think that if there is an error, it will always throw an error. and once the user resets the error boundary and thus rerenders the component that had the suspended query that threw the error, it will refetch again

@FaberVitale
Copy link
Contributor Author

Thank you @markerikson @wuweiweiwu for your suggestions and sorry for the delayed reply:

I generally don't do open source development during working days.


An interesting comment from Reactiflux:

at this point I think const { data } = useSuspendedThing() is actually an antipattern.

It breaks down when you need to query things in parallel

I agree.

Suggested alternatives

1. result.read()

const { readThing1 } = useSuspendedThing1(); // start loading
const { readThing2 } = useSuspendedThing2();

const thing1 = readThing1(); // select data
const thing2= readThing2();

This pattern is a pandora's box of potential misuses:

1.a Accidental waterfalls
const { readThing1 } = useSuspendedThing1(); 
const thing1 = readThing1(); // oops waterfall!
const { readThing2 } = useSuspendedThing2();

const thing2= readThing2();
1.b Accidental exception thrown during effect

This pattern is really problematic if there's a closure on the accessor function (readThing).

const { readThing1 } = useSuspendedThing1(); 

useEffect(() => {
  if(checkConditionOn(prop1, prop2)) {
    setValue(readThing1() ) // It could throw a promise or an error here
  }

}, [prop1, prop2]);
2 SWR RFC vercel/swr#168

I deem this approach hackish because it breaks the rule of hooks.


Given your feedback I'm not sure that it makes sense to keep on going with this design and I'd like to try to tackle the problem with a different approach.

useSuspendQueries

useSuspendQueries a generic hook that suspends queries and that could be used to suspend other type of hooks;
Interally throws a Promise.all.

Usage

const [{ data: query1Res }, { data: query2Res }] = useSuspendQueries(
  useQuery1(arg1),
  useQuery2(arg2),
);

I'll attempt this approach in a new PR.

@phryneas
Copy link
Member

phryneas commented Mar 26, 2022

Just as a small api suggestion, maybe more something in the direction

const [{ data: query1Res }, { data: query2Res }] = useSuspendQueries(
  // no hooks "passed in elsewhere" - that can easily break rules of hooks in unintended ways
  [endpoint1, arg1],
  [endpoint2, arg2],
);

Although, tbh, I'm not really sure about that approach.

at this point I think const { data } = useSuspendedThing() is actually an antipattern.

I honestly think it is okay - many components just query one api and everything we do here could only prevent per-component waterfalls. So it might make sense to just put more focus on prefetching.

The simple use case (one fetch in the component, we do not care too much about cross-component waterfalls) could just be

const [data, { refetch }] = useSuspendedThing1(arg1) 

While prevening an in-component-waterfall would look like

function MyComponent(){
  usePrefetchThing1(arg1) 
  usePrefetchThing2(arg2) 

  const [data1, { refetch }] = useSuspendedThing1(arg1) 
  const [data2, { refetch }] = useSuspendedThing2(arg2) 

  return <>...</>
}

or, if something like react-location or the new react-router version is used we can prevent cross-component waterfalls:

// in router:
ensureThingFetching1(arg1) 
ensureThingFetching2(arg2) 

// in component
function MyComponent(){
  const [data1, { refetch }] = useSuspendedThing1(arg1) 
  const [data2, { refetch }] = useSuspendedThing2(arg2) 
}

@markerikson
Copy link
Collaborator

markerikson commented Mar 26, 2022

@wuweiweiwu
Copy link
Contributor

I honestly think it is okay - many components just query one api and everything we do here could only prevent per-component waterfalls. So it might make sense to just put more focus on prefetching.

agreed. To achieve true render-as-you-fetch you'd always have to decouple the initiation of the fetch (prefetching) and the reading of the data I believe.

@drcmda pointed out https://github.com/pmndrs/suspend-react and https://github.com/pmndrs/jotai/blob/main/docs/basics/async.mdx, which might be useful API examples

those apis seems to be inline with what relay and recoil exposes as well

@FaberVitale
Copy link
Contributor Author

Just as a small api suggestion, maybe more something in the direction

const [{ data: query1Res }, { data: query2Res }] = useSuspendQueries(
  // no hooks "passed in elsewhere" - that can easily break rules of hooks in unintended ways
  [endpoint1, arg1],
  [endpoint2, arg2],
);

I do not get why useSomething(useThat()) breaks the rule of hooks.

The solution I've proposed does not break the rule of hooks because useQuery1 and useQuery2 are just standard buildHooks.useQuery that don't throw:

my idea is to add a simple getSuspendablePromise(): Promise<unknown> | null | undefined method to the queryResult that would return a promise if there's a pending request or null or undefined of it should not trigger a fetch.

It is a simple contract that would allow to suspend non rtk-query resources.

Although, tbh, I'm not really sure about that approach.
[...]
I honestly think it is okay - many components just query one api and everything we do here could only prevent per-component waterfalls. So it might make sense to just put more focus on prefetching.
[...]

I'm not sure about this: In my experience it is rather common to have a component that relies on at least 2 distinct data sources.

While prevening an in-component-waterfall would look like

function MyComponent(){
  usePrefetchThing1(arg1) 
  usePrefetchThing2(arg2) 

  const [data1, { refetch }] = useSuspendedThing1(arg1) 
  const [data2, { refetch }] = useSuspendedThing2(arg2) 

  return <>...</>
}

or, if something like react-location or the new react-router version is used we can prevent cross-component waterfalls:

// in router:
ensureThingFetching1(arg1) 
ensureThingFetching2(arg2) 

// in component
function MyComponent(){
  const [data1, { refetch }] = useSuspendedThing1(arg1) 
  const [data2, { refetch }] = useSuspendedThing2(arg2) 
}

I see those prefetch calls as avoidable boilerplate code:
they add nothing, they are just there to provide more cognitive load and sloc.


Conclusion

@phryneas has suggested a reasonable alternative to the version of useSuspendQueries that I've proposed in #2149 (comment).

I'll tinker a bit with these 2 api signatures and then I'll weight pros and cons of their implementations.

@phryneas
Copy link
Member

phryneas commented Mar 26, 2022

I do not get why useSomething(useThat()) breaks the rule of hooks.

Ah sorry, I had misread your example! I thought you were passing the hook in, not the hook result.

In the end, just keep exploring - in the end I'm just watching from the sidelines here at the moment :)

@drcmda
Copy link

drcmda commented Mar 27, 2022

Just curious, but "unstable" ? Suspense has been part of React since 16.6, no difference between React.lazy and useQuery. React 18 intents to ship, they're just fixing docs.

@phryneas
Copy link
Member

phryneas commented Mar 27, 2022

@drcmda suspense for data fetching is not part of the initial React 18 release and "how it should be used" has still not be fully fleshed out by the React team.

It would be foolish to go with an api now that we might have to scrap later because they invent some pattern we don't know about yet.

@drcmda
Copy link

drcmda commented Mar 27, 2022

imo suspense for lazy loading and "suspense for data fetching" are one and the same, you throw a promise. even the truly experimental parts like <Cache> or how bounds trigger render and in what order are just user land semantics. but i was just curious about the name, no problem at all.

@phryneas
Copy link
Member

@drcmda As I interpret the statements of the React team, suspense for lazy loading is a subset of what suspense for data fetching is supposed to be - but afaik the public just doesn't know all the details for it yet.

@drcmda
Copy link

drcmda commented Mar 29, 2022

@drcmda As I interpret the statements of the React team, suspense for lazy loading is a subset of what suspense for data fetching is supposed to be - but afaik the public just doesn't know all the details for it yet.

suspense is a thrown promise, nothing more. react just re-renders when it's resolved. in order to not cause a loop you cache. they may or may not give us tools to handle that part in the future.

either way, https://reactjs.org/blog/2022/03/29/react-v18.html#suspense-in-data-frameworks

In React 18, you can start using Suspense for data fetching in opinionated frameworks like Relay, Next.js, Hydrogen, or Remix. Ad hoc data fetching with Suspense is technically possible, but still not recommended as a general strategy.

with next using suspense in prod nothing's going to change how it fundamentally works. :-)

@FaberVitale
Copy link
Contributor Author

Update

I'm making good progress with the api I've proposed in #2149 (comment)

I hope to have it ready next weekend.

Screenshot 2022-04-03 at 23 14 09

@PupoSDC
Copy link

PupoSDC commented Apr 10, 2022

Just want to give a huge "+1" to @FaberVitale on this one. I implemented this as an over the top solution for my project and it works great!!

The only problem is that due to the type generation used by this library it's all but impossible to generalize this so that I don't have to write a wrapper for each method i want to suspense (which is all of them actually).

Here is a snippet of the code:

export const createSuspendedQueries = <
  A extends BaseQueryFn<any, unknown, unknown, {}, {}>, 
  B extends EndpointDefinitions, 
  C extends string, 
  D extends string
  >(api: Api<A, B, C, D>) => {
  return Object
    .keys(api.endpoints)
    .filter((e) : e is any => !!((e as any).useQuery))
    .map((name) => {
      const endpoint = api.endpoints[name];
      const a : typeof endpoint["useQuery"] = (args) => {
        const endpoint = api.endpoints[name];
        const dispatch = useDispatch()
        const queryOutput = (endpoint as any).useQuery(args);

        const readData = () => {
          if (queryOutput.error) {
            throw queryOutput.error;
          }
      
          if (!queryOutput.isFetching && queryOutput.data) {
            return queryOutput.data
          }
      
          let promise = queryQuestionBank.util.getRunningOperationPromise(name, args)
      
          if (promise) {
            throw promise.unwrap()
          }
      
          dispatch(
            queryQuestionBank.util.prefetch("contentTree", args, { force: true })
          )
      
          promise = queryQuestionBank.util.getRunningOperationPromise("contentTree", args)
          if (promise) {
            throw promise.unwrap()
          }
      
          throw new Error("we goofed")
        };

        return {
          ...queryOutput,
          readData
        }
      }
    })
}

this works all right as far as JS goes, the TS types are all screwed up though.

Can't wait for this to make it to master: it's a game changer as far as writing next js apps goes 😄

@FaberVitale
Copy link
Contributor Author

I've just uploaded a second POC that is based on #2149 (comment)

Thank you for your feedback :)

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

Successfully merging this pull request may close these issues.

6 participants