feat: add useSuspendAll hook & react/suspense example #2245

FaberVitale · 2022-04-16T11:44:24Z

Description

Proof of concept of a suspense for data fetching implementation.
It is based on useSuspendAll, a custom hook that parallelizes suspended queries and outputs
input arguments with remapped TS types.

Demo

Issues

Closes #1283 #1574

Usage

1. Simple single query

  const [{  data, isLoading }] = useSuspendAll(
    useGetPokemonByNameQuery(name),
  )

 //  data is defined here, isLoading is false & isSkipped is boolean

2. Skipped Query

 import { skipToken } from '@reduxjs/toolkit/query/react';
 
  const [{  data, isLoading }] = useSuspendAll(
    useGetPokemonByNameQuery(skipToken),
  )

 //  data here is Data | undefined, isLoading is boolean and isSkipped is true

3. Parallel queries

  const [baseDataQuery, evolutionQuery] = useSuspendAll(
    useGetPokemonByNameQuery(base),
    useGetPokemonByNameQuery(evolution)
  )

 //  baseDataQuery.data is defined here
 //  evolutionQuery.dara is defined here

4. Error Handling

import * as React from 'react'
import { ErrorBoundary } from 'react-error-boundary'
import Comp from 'components/Comp';

function CompWithErrorBoundary(props) {
  return (
    <ErrorBoundary
      fallbackRender={({ resetErrorBoundary, error }) => (
        <div onClick={() => { 
         (error as any)?.retryQuery?.()
          resetErrorBoundary()
       }}>retry</div>)}>
      <React.Suspense
        fallback={<div>loading ...</div>}
      >
        <Comp {...props} />
      </React.Suspense>
    </ErrorBoundary>
  );
}

Technical details

useSuspendAll expects one or more objects that implement the Suspendable interface as arguments:

export interface Suspendable {
  /** 
   * Returns a promise if there's a pending request, 
   * throws error in case of unsuccessful request
   * or returns undefined
   */
  getSuspendablePromise(): Promise<unknown> | undefined
}

useSuspendAll returns the input arguments in array and does not mutate them, it uses instead TS property remapping
change TS output if input arg extends SuspendableResource:

export interface Resource<Data> {
  data?: Data | undefined
  isLoading?: boolean
}

export type SuspendableResource<Data> = Resource<Data> & Suspendable

Changes to existing APIs.

Adds the following properties to api.endpoints.useQuery output:

getSuspendablePromise
isSkipped: true if query is skipped
adds keepSubscriptionFor to prefetchOptions
adds keepPrefetchSubscriptionsFor to api.config (default 10s)
prefetch subscriptions are now removed after prefetchOptions.keepSubscriptionFor, if provided,
or api.config.keepPrefetchSubscriptionsFor otherwise.
Prefetch Thunk and usePrefetch previously returned undefined now return PrefetchActionCreatorResult:

interface PrefetchActionCreatorResult {
  /**
   * Returns a promise that **always** resolves to `undefined`
   * when there are no pending requests initiated by input thunk.
   */
  unwrap(): Promise<void>,
  /**
   * Cancels pending requests.
   */
  abort(): void
}

Internal changes

prefetch queries now have the same requestId and the same subscription key

New Apis

Todo

React 18 testing
Figure out why err instanceof SuspenseQueryError doesn't work
Add more tests
Handle edge cases
Should we suspend other rtk-query hooks (e.g. useLazyQuery)?

Alternatives

Similar apis

jotail waitForAll

Previous attempts

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

codesandbox-ci · 2022-04-16T11:48:07Z

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 65ba8a3:

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

packages/toolkit/src/query/react/exceptions.ts

packages/toolkit/src/query/react/buildHooks.ts

phryneas · 2022-04-16T20:23:46Z

packages/toolkit/src/query/react/buildHooks.ts

+          if (!pendingPromise) {
+            throw new Error(
+              `[rtk-query][react]: invalid state error, expected getRunningOperationPromise(${name}, ${queryStateResults.requestId}) to be defined`
+            )
+          }


Do you see this ever happening? Also, are we saving another promise than prefetch returns into the pending promises? Otherwise we could reuse that.

Do you see this ever happening?

No , it's an invariant that was useful while iterating on this feature. I can remove it.

Also, are we saving another promise than prefetch returns into the pending promises? Otherwise we could reuse that.

usePrefetch() returns a consumer:

const prefetch: (arg: any, options?: PrefetchOptions | undefined) => void

I do not think that I can extract a promise from that.

I've skimmed a bit the type of api.util.prefetch and it seems that has type:

<EndpointName extends QueryKeys<Definitions>>( endpointName: EndpointName, arg: any, options: PrefetchOptions ): ThunkAction<void, any, any, AnyAction>

How can I extract a promise from that thunk action, I've not found a test that unwraps a promise from api.util.prefetch.

Can you please provide more details?

We would need a return dispatch here everywhere this has only a dispatch to get that promise reliably:

redux-toolkit/packages/toolkit/src/query/core/buildThunks.ts

Lines 453 to 468 in e7a98eb

dispatch(queryAction())

} else if (maxAge) {

const lastFulfilledTs = latestStateValue?.fulfilledTimeStamp

if (!lastFulfilledTs) {

dispatch(queryAction())

return

}

const shouldRetrigger =

(Number(new Date()) - Number(new Date(lastFulfilledTs))) / 1000 >=

maxAge

if (shouldRetrigger) {

dispatch(queryAction())

}

} else {

// If prefetching with no options, just let it try

dispatch(queryAction(false))

Perfectly viable to just add that.

MEMO: It's a breaking change

returning something from a function that returned undefined before? I wouldn't really call that breaking

prefetch thunk and usePrefetch callback now return an object.

interface PrefetchActionCreatorResult { /** * Returns a promise that **always** resolves to `undefined` * when there are no pending requests initiated by input thunk. */ unwrap(): Promise<void>, /** * Cancels pending requests. */ abort(): void }

packages/toolkit/src/query/react/buildHooks.ts

phryneas · 2022-04-16T20:28:48Z

packages/toolkit/src/query/react/buildHooks.ts

+          name
+        )
+
+        const prefetch = usePrefetch(name as any)


using prefetch here might be problematic since that creates a value subscription that cannot be unsubscribed. if we use that here we have to look into that

You're right :/

I just need a way to fetch once and then to store the result.

I think at this point we probably need to overwork prefetch so that it takes something like a "keepFor" timer to add a time-based temporary subscription (maybe with a fixed subscription id of "prefetch" so that multiple prefetches just "bump up the timer").

I hate to bother you to go deeper into this rabbit hole - but could you look into this?

Done f994418 :)
Please let me know what you think
I'm still not sure about the suggested changes to prefetch return type but I'll look into it.

packages/toolkit/src/query/react/exceptions.ts

packages/toolkit/src/query/react/suspense-utils.ts

phryneas · 2022-04-16T20:36:31Z

packages/toolkit/src/query/react/suspense-utils.ts

+    .filter(isPromiseLike) as Promise<unknown>[]
+
+  if (promises.length) {
+    throw Promise.all(promises)


That would create a new promise each time, but we need to keep it stable. Maybe just throw the first unresolved promise? (We would need to add an isResolved property to the promises in that case)

That would create a new promise each time, but we need to keep it stable. Maybe just throw the first unresolved promise? (We would need to add an isResolved property to the promises in that case)

I've not seen suspense for data fetching implementation that relies on isResolved or similar.

That would create a new promise each time

Yes but it is not a problem because you are suspending the rendering until all the promises are resolved and
runningQueries does not keep promises that are settled.

Moreover it is the same approach used by joitai waitForAll

I remember reading in the working group that for suspense it is important to always get the same promise back, but right now I can't find the quote for it.

Yep, pretty sure I saw that too, but don't immediately have a pointer either.

Yep, pretty sure I saw that too, but don't immediately have a pointer either.

I'll look into it 👍 .

I've got a couple of ideas on how to tackle this issue but I'd like to migrate the examples to react 18 first.

phryneas · 2022-04-16T20:40:52Z

This looks like a great approach, but with prefetch you have hit a hornet's nest of an old TODO that needs to be solved better ;)

I have skimmed the source and left some comments, but I have spent all day recording egghead video lessons, so my brain is mush. Very likely that some comments don't make sense or I missed some things.

But please keep going! 👍

Feedback: - reduxjs#2245 (comment) - reduxjs#2245 (comment) - reduxjs#2245 (comment) - reduxjs#2245 (comment) - reduxjs#2245 (comment)

…mer reduxjs#1283 Closes reduxjs#1283 Automatically removes prefetch subscriptions after configurable amount of time. Description: Prefetch subscription are now removed after `prefetchOptions.keepSubscriptionFor` if provided or api.config.keepPrefetchSubscriptionsFor otherwise. Api changes: - adds `keepSubscriptionFor` to prefetchOptions - adds `keepPrefetchSubscriptionsFor` to api.config (default 10s) Internal changes: - prefetch queries now have the same requestId and the same subscription key

…ePromise

Description: prefetch thunk now returns an object with shape: ```ts export interface PrefetchActionCreatorResult { /** * Returns a promise that **always** resolves to `undefined` * when there are no pending requests initiated by input thunk. */ unwrap(): Promise<void>, /** * Cancels pending requests. */ abort(): void } ```

netlify · 2022-06-11T08:53:18Z

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

Name	Link
🔨 Latest commit	`65ba8a3`
🔍 Latest deploy log	https://app.netlify.com/sites/redux-starter-kit-docs/deploys/62c04dcedecd93000849807e
😎 Deploy Preview	https://deploy-preview-2245--redux-starter-kit-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

Hypnosphi · 2022-06-27T15:41:20Z

packages/toolkit/src/query/core/buildThunks.ts

+          unwrap() {
+            return initiateOutput.unwrap().then(noop, noop)
+          },
+          abort: initiateOutput.abort,


what about unsubscribe? Why not just return initiateOutput as is?

initiateOutput returns QueryActionCreatorResult

type QueryActionCreatorResult< D extends QueryDefinition<any, any, any, any> > = Promise<QueryResultSelectorResult<D>> & { arg: QueryArgFrom<D> requestId: string subscriptionOptions: SubscriptionOptions | undefined abort(): void unwrap(): Promise<ResultTypeFrom<D>> unsubscribe(): void refetch(): void updateSubscriptionOptions(options: SubscriptionOptions): void queryCacheKey: string }

It returns methods that do not fit the prefetch model like updateSubscriptionOptions and can cause more harm than anything else if misused.
I just want to expose the informations strictly useful.

Moreover we can always expose later more properties of initiate output if necessary, speaking of which we should at least expose arg and queryCacheKey.

Hypnosphi · 2022-06-27T15:44:04Z

packages/toolkit/src/query/core/buildThunks.ts

+
+        return {
+          unwrap() {
+            return initiateOutput.unwrap().then(noop, noop)


looks like this way you remove information about both fulfilled and rejected values

Yeah it makes sense to at least return the output of unwrap

Hypnosphi · 2022-07-01T00:19:10Z

Also, initialize accepts {subscribe?: boolean} option so maybe prefetch should as well

FaberVitale · 2022-07-02T13:03:18Z

Also, initialize accepts {subscribe?: boolean} option so maybe prefetch should as well

I'm afraid I do not agree with this change.

The whole point of prefetch is to put data in the store because you anticipate that you are going to query later.
It wasn't part of the prefetch api previously and I do not believe that it should be added with these changes.

Moreover there's already initiate(arg, { subscribe: false }) if the user doesn't want put fetched data in the store.

FaberVitale · 2022-07-02T13:11:15Z

Prefetch PR

I'd like to move the prefetch changes (see #1283) in another PR.
This one has become a bit too big and it is hard to follow, review and develop.
I plan to keep on developing this PR once the prefetch changes get merged.

…o feat/use-suspend-all

Hypnosphi · 2022-07-03T12:43:47Z

Sorry, I didn't realize {subscribe: false} doesn't put data in the store, I thought it only opts out of automatic refetching. Is there any mechanism to do the latter without the former?

markerikson · 2022-08-12T01:11:02Z

FYI, the React team just put up a PR that looks to add what might eventually be "official" support for Suspense triggering: experimental_use(promise).

It still throws internally, but only after introspecting the promise:

facebook/react#25084

the PR says throwing directly would eventually be deprecated

FaberVitale · 2022-08-15T06:56:33Z

[...]
facebook/react#25084

the PR says throwing directly would eventually be deprecated

Caching promises

Another feature is that the promises do not need to be cached between attempts. Because we assume idempotent execution of components, React will track the promises that were used during the previous attempt and reuse the result. You shouldn't rely on this property, but during initial render it mostly just works. Updates are trickier, though, because if you used an uncached promise, we have no way of knowing whether the underlying data has changed, so we have to unwrap the promise every time. It will still work, but it's inefficient and can lead to unnecessary fallbacks if it happens during a discrete update.

A shallow equalilty check of the pending promises list should be enough to avoid issues during updates.

markerikson · 2022-09-07T20:54:22Z

Kevin Ghadyani put up a vid with him talking to Tanner about how React Query does Suspense.

Haven't even watched it yet, but throwing it here for reference:

https://twitter.com/Sawtaytoes/status/1567606755214983170