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

Adding React suspense + data fetching support #9627

Closed
Tracked by #366
hwillson opened this issue Apr 26, 2022 · 42 comments
Closed
Tracked by #366

Adding React suspense + data fetching support #9627

hwillson opened this issue Apr 26, 2022 · 42 comments

Comments

@hwillson
Copy link
Member

hwillson commented Apr 26, 2022

Broken out from: apollographql/apollo-feature-requests#366

Now that @apollo/client 3.6 is out with updated React 18 support, this issue will track Apollo Client's React suspense + data fetching support (coming in 3.7).

@capaj
Copy link
Contributor

capaj commented May 6, 2022

@hwillson I am slightly confused-is react supporting suspense for data fetching?
Looking at the docs: https://reactjs.org/docs/react-api.html#reactsuspense

It's not mentioned anywhere. They only explicitly list these three usecases:

  • code splitting
  • server side rendering
  • hydration

Although looking at how urql team handles it-it seems that just throwing a promise anywhere should be enough for suspense to just work, am I correct in this assumption?

@SimenB
Copy link
Contributor

SimenB commented May 6, 2022

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

@capaj
Copy link
Contributor

capaj commented May 6, 2022

@SimenB thanks that explains it well enough.

@pkellner
Copy link
Contributor

Does this issue imply that there will be a data fetching solution when using Suspense in React 18 for Apollo? That is, something like useQuery will be updated (though I think there needs to be support beyond useQuery). I've attempted to start a discussion here

@jpvajda
Copy link
Contributor

jpvajda commented May 11, 2022

@pkellner Just an update on the use of Github Discussions in this repo, we are actually disabling the use of Discussions on May 18th, so if you want just keep your comments on this issue, that works well for us. We are happy to respond when we can. 😄

As a note: Apollo has centralized discussions over in our Community Forum so we can better monitor them and respond across our many OSS projects. Feel free to join the community if you find value in it.

@benjamn
Copy link
Member

benjamn commented May 11, 2022

@pkellner We can definitely keep discussing here, to be clear!

And yes, we do plan either to provide additional suspenseful versions of the relevant hooks, or (if @capaj's question above turns out in our favor 🤞), then perhaps we can just make the existing hooks return (for example) useQuery(...).suspensePromise, which would be the same Promise that would be thrown during suspense, and if you call useQuery(query, { suspend: true }) it automatically throws that suspensePromise, or something along those lines. On the other hand, a thin wrapper around useQuery might be adequate too.

@pkellner
Copy link
Contributor

pkellner commented May 11, 2022

@benjamn glad to hear from you on this. I totally get how the simple sample works on the React Blog works with Suspense. That is, throwing the status, or returning the result. I also believe, based on @gaearon comment here, that it is unlikely that useQuery is able to support Suspense. That is, Dan says in this issue "We're not sure whether ad-hoc helpers like useSWR will ever be recommended to use with Suspense" making me think there must be a lot of locking/race type issues that need to be addressed, well beyond simple throws in a wrapped promise.

I really hope you at Apollo figure out how to make Suspense work with data. You are are in a perfect position with your well tested, well documented, and fully supported library to bring Suspense to lots and lots of people.

And also, @jpvajda , thanks for the note about discussions being disabled. I did not realize that and I will continue this discussion here.

@MarkLyck
Copy link

@hwillson and @benjamn Thanks for creating this public issue and commenting.

I was just about to start a huge painful migration from @apollo-client to another graphql client to get Suspense support, as apollo is the last library i have really preventing me from taking full advantage of React 18.

So happy to hear that Suspense hooks are coming!

@pkellner
Copy link
Contributor

@hwillson and @benjamn Thanks for creating this public issue and commenting.

I was just about to start a huge painful migration from @apollo-client to another graphql client to get Suspense support, as apollo is the last library i have really preventing me from taking full advantage of React 18.

So happy to hear that Suspense hooks are coming!

Relay?

@MarkLyck
Copy link

MarkLyck commented May 20, 2022

@pkellner I was going to give gqty a try. I built a POC for it, It's amazing being able to just write the code as if it was an object and not have to think about queries at all. But I could not get a decent dynamic mocking system working since the queries don't have names. I would essentially have to rely on graphql-tools auto-mocking + using Type names for specific overrides. Another downside is no support for @defer or @stream directives. But graphql-mesh doesn't support that yet anyway 😞

with Apollo I use graphql-ergonomock for auto generated mocks, and overriding them for individual tests which just works great!

My backup would have been react-query or Urql (all 3 of those have had Suspense support for a long time), I've never really liked Relay, much prefer Apollo client.

@pkellner
Copy link
Contributor

Thanks! Obviously mocking is very important to you. I get it.

Be very careful if libraries that claim proper Suspense support. I started this issue and Dan weighed in making it clear the only current properly working data library with Suspense is relay. Definitely SWR does not support Suspense correctly. They've talked about removing the option but I'm not sure why they have not.

vercel/swr#1906 (comment)

@MarkLyck
Copy link

MarkLyck commented May 20, 2022

@pkellner Interesting 🤔 My app is client-side rendered, so I don't think this particular issue would affect my application.

I've been using react-query's implementation of Suspense in production for a while now and so far it has been working flawlessly.(Of course knowing that the final APIs could change at some point). But so far, no errors or any data issues with it.

But good to know in case I implement this on one of my SSR apps 😅

@pkellner
Copy link
Contributor

I'll ping Dan, but I don't think it is a server side/client side issue. I think that Suspense is unstable in production on client side with anything but relay. Otherwise, they would endorse Suspense with CRA and they clearly don't do that.

@pkellner
Copy link
Contributor

pkellner commented May 23, 2022

@MarkLyck Not sure if you are up for taking risks with your production but... Seems to me like it would be foolish to use Suspense on any kind of production web site until the React team endorses its use. Dan replied.. vercel/swr#1906 (comment)

To be clear, I posted this to twitter and Dan responded here.

https://twitter.com/pkellner/status/1528852128160423936

@jpvajda jpvajda modified the milestones: Release 3.7, Release 3.8 May 24, 2022
@doflo-dfa
Copy link

We have been using the following pattern after split control approach (after admittedly trying a number of other apollo wrapper approaches .... this has proven by far the most predictable give the flux everything is in )

export const ControlName = ()=>{

const query = useQuery<any>(QUERY)

return <Suspense><ControlNameRenderer query={query} /></Suspense>

}

const ControlNameRenderer = (props: { query: QueryResult<any> })=>{

  if (props.query.loading) {
    throw Promise.resolve();
  }
  
  if (props.query.error) {
    throw props.query.error;
  }

  return <div>{props.query.data.value}</div>
  
}

default export memo(ControlName)

@laverdet
Copy link
Contributor

We have been using the following pattern after split control approach (after admittedly trying a number of other apollo wrapper approaches .... this has proven by far the most predictable give the flux everything is in )

export const ControlName = ()=>{

const query = useQuery<any>(QUERY)

return <Suspense><ControlNameRenderer query={query} /></Suspense>

}

const ControlNameRenderer = (props: { query: QueryResult<any> })=>{

  if (props.query.loading) {
    throw Promise.resolve();
  }
  
  if (props.query.error) {
    throw props.query.error;
  }

  return <div>{props.query.data.value}</div>
  
}

default export memo(ControlName)

This will peg the CPU at 100% while any queries are loading.

@doflo-dfa
Copy link

doflo-dfa commented May 30, 2022

I may have accidentally left out the promise helper we are using inplace of the generic Promise.resolve() ... cause I am dumb

ill post a better example

@jpvajda jpvajda added this to the Release 3.8 milestone Jun 16, 2022
@jpvajda jpvajda modified the milestones: Release 3.8, Release 3.9 Jul 26, 2022
@Negan1911
Copy link

@pkellner I think apollo is not an "ad hoc type helper" like SWR, but a data library like relay, so suspense should be possible as dan says here: vercel/swr#1906 (comment)

@rajington
Copy link
Contributor

rajington commented Aug 11, 2022

Definitely outdated so not sure if it's relevant, or had it been mentioned now something like useSWR would be included, but in the v17 docs Apollo is mentioned directly.

I assume bc Apollo has that intermediate caching layer, it has the potential to be leveraged by React, but I would also assume that the integration doesn't come for free and that things are constantly evolving, case in point the v18 docs are very different.

I wish I understood more around the complexities and types of issues we might run into now. We experimented with Suspense for data fetching on a non-critical (yet highly trafficked) production page via (3rd-party deprecated) react-apollo-hooks more than 3 years ago! Then later we even PoC'd it with Next.js using react-async-ssr/react-ssr-prepass/getDataFromTree from early versions of React 18 experimental. In both cases, the components we were able to write were beautiful, which is what makes me excited about being able to use this more confidently throughout our sites. Likely we were not reaching some edge cases, but it'd be great to know if there's a certain type of data (maybe immutable, fetched once data) that it could work for.

@NTag
Copy link

NTag commented Aug 11, 2022

Hello 👋

Do you have any update on this topic? Do you still plan to include it in 3.7 or will it be in a later release?

Thanks for the work!

@jpvajda
Copy link
Contributor

jpvajda commented Aug 15, 2022

@NTag Thanks for checking in on this feature. We have moved it to our 3.9 release due to other priorities that came up for the team recently. We are very interested in delivering support for suspense very soon to Apollo Client. So thanks for being patient. 🙏

@jamesmfriedman
Copy link

jamesmfriedman commented Sep 2, 2022

Wrote this today, surprisingly it works as a drop in replacement to useQuery with improved ergonomics.

  • Use inside of a component in a React.Suspense component, works as expected
  • data is no longer possibly undefined, so you can access it without the short circuit operator in your views.
  • I didn't like any of the examples I found online of having to do data accessor like data.foo.read() so I'm using a proxy. Just access data like you normally would.
  • If anyone is seeing something I'm not, please @ me, otherwise easier than I expected.

Implementation

import { useMemo } from 'react';

import {
  OperationVariables,
  DocumentNode,
  TypedDocumentNode,
  QueryHookOptions,
  QueryResult,
  useQuery,
} from '@apollo/client';

/**
 * This is a drop in replacement for Apollo's useQuery hook that works directly
 * with React.Suspense and has the improved ergonomics of `data` being non-nullable.
 */
export const useSuspenseQuery = <
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  TData extends any = any,
  TVariables extends OperationVariables = OperationVariables
>(
  query: DocumentNode | TypedDocumentNode<TData, TVariables>,
  options: QueryHookOptions<TData, TVariables> = {}
): Omit<QueryResult<TData, TVariables>, 'data'> & {
  data: NonNullable<QueryResult<TData, TVariables>['data']>;
} => {
  const { data, loading, error, observable, ...rest } = useQuery(query, options);

  const errorPolicy = options.errorPolicy || 'none';

  const promise = useMemo(() => {
    return new Promise((resolve) => {
      const resolver = () => {
        resolve(true);
        subscription.unsubscribe();
      };
      const subscription = observable.subscribe(({ data, loading }) => {
        data && !loading && resolver();
      });
    });
  }, [observable]);

  const proxy = useMemo(
    () =>
      new Proxy((data || {}) as object, {
        get: (target, prop) => {
          if (!Object.keys(target).length && loading) {
            throw promise;
          } else if (errorPolicy === 'none' && error) {
            throw error;
          }
          return target[prop as keyof typeof target];
        },
      }) as NonNullable<TData>,
    [data, loading, error, errorPolicy, promise]
  );

  return { data: proxy, loading, error, observable, ...rest };
};

Usage

function Example() {
  <React.Suspense fallback={'My Fallback'}>
    <MyComponent />
  </React.Suspense>
}

function MyComponent() {
  const { data } = useSuspenseQuery(MyQuery);
  return <>{data.someValue}</>
}

@pkellner
Copy link
Contributor

pkellner commented Sep 2, 2022

I believe that the Facebook and react team will tell you that without underlying support for the promise completion in Apollo that suspense will not be reliable and it will be suggested not to use your solution in production. True @benjamn and @gaearon

@jamesmfriedman
Copy link

Maybe, there is no Apollo promise in this case, it's just queued off of the Apollo observable which is triggering a standard / memoized promise. Would love to know what I don't know though 🤓.

@pkellner
Copy link
Contributor

pkellner commented Sep 2, 2022

As would I. Suspense and data is a very unclear story. I do know that straight forward solutions like yours proposed are in the category of problematic with suspense.

@Negan1911
Copy link

@pkellner not really, on the past we had apollo hooks with something similar to that, if the solution is reliable why it would cause problems?

@pkellner
Copy link
Contributor

pkellner commented Sep 2, 2022

@Negan1911 the issue is specific to Suspense. I've had a lot of discussions about this in different issues.

The key part of these are referencing "ad-hoc libraries like.."

Here are some links

vercel/swr#1906 (comment)

vercel/swr#1906 (comment)

facebook/react#23045 (comment)

@Negan1911
Copy link

@pkellner as I said to you previously, Apollo is not an "ad hoc" library but a full fledged client like Relay.

Like dan said:

Suspense is only ready for usage in data frameworks (like Relay), not in ad-hoc helpers like useSWR

Anyways, I don't see the point of tagging people and turn down alternatives published by people.

@pkellner
Copy link
Contributor

pkellner commented Sep 2, 2022

@Negan1911 I tagged Ben and Dan because they are both very involved in the success of Suspense and your proposed solution is exactly the kind of thing that I would love to see verified and ready for production. I'm only commenting here because there is a lot of confusion right now as to what people (like me and you) believe will work in production and will not work in production. I think it's completely reasonable that you assume that your solution is 100% solid and should be used in production. The bigger, and frankly, more worrisome issue, is that it is not likely recommended for production with Suspense in the current released version of React 18.2. I assume you posted this because you wanted feedback. If you think my feedback is in appropriate, LMK and I'll delete all my comments.

@jamesmfriedman
Copy link

I appreciate the discourse, other than manual testing, unit testing, and digging through React's source I'm not really sure how to suss out if this is or isn't ok for production, so hopefully Ben and Dan can chime in and shed some light on why this is a good or terrible idea.

@laverdet
Copy link
Contributor

laverdet commented Sep 5, 2022

@jamesmfriedman you subscribe to the observable but you never unsubscribe. I think that as your app continues to run you will accumulate orphaned subscriptions. You may be tempted to unsubscribe in a useEffect hook destructor but it is possible react will invoke your render function but then abandon the render due to a transition. In that case the component may never actually mount or unmount. So you'll need some kind of timer-based heuristic to detect these abandoned renders, and the cracks in this implemention will begin to show.

@jamesmfriedman
Copy link

Yep, I already caught the unsubscribed observable problem after I posted @laverdet, but the rest of what you describe seems like it would be a problem for the internals of Apollo as well since I'm presuming they use their own observable.

In terms of React not calling the cleanup function of a hook, I can't find any evidence or resources indicating that behavior. If that is a behavior of React (that it can unmount and never run the hook destructor), memory leaks would be problematic for all sorts of different usecases, not just this one.

@laverdet
Copy link
Contributor

laverdet commented Sep 6, 2022

@jamesmfriedman I think you misunderstood my comment. I'm not saying that the destructor will be skipped, I'm saying that the component will never mount, and therefore it cannot unmount. If your render function has a side-effect it is impossible to undo that side-effect reliably. That's why the React team has been screaming from the rooftops that render functions must be pure. This is the culmination of like 4 years of work from the React team on fibers, concurrent mode, and suspense.

This sample should demonstrate the situation clearly:
https://codesandbox.io/s/react-18-playground-forked-kj4m8d

If you look at the console you'll see that WithSuspense renders, but never mounts because that render path has been abandoned. If you modify the 1000ms timer to 100ms then it will mount and unmount. You can also get the same result without using suspense at all but this example is easiest. React is allowed to call your render function as many times as it wants without actually mounting.

@jamesmfriedman
Copy link

Thanks for the example @laverdet. I'm only using memoized values in my particular case and am now unsubscribing from the observable on promise resolution. I don't believe I'll end up in the condition you posted about since I'm not relying on any useEffect hook. I do appreciate the example you posted though. We'll likely use the hook as implemented above and monitor it for any abnormalities. I suspect if there are any potential issues they would memory leak related. I'll definitely repost if we see any sort of unintended consequences from using it.

@idonov
Copy link

idonov commented Sep 6, 2022

@jamesmfriedman Can I get some clarification what the use flow is with Suspense(hacks or when Apollo 3.9 comes out).
I assume with Suspense we don't use getDataFromTree anymore, correct? Otherwise it would beet the point of Suspense.
Anyway I experimented a bit and I am curious, when I remove getDataFromTree my useQuery is always loading and never finishes on the server. Is this normal, or am I missing something?

@hrougier
Copy link

Based on @jamesmfriedman solution, one could make a useQuery.ts with suspense support

const { data, error } = useQuery(query, { suspense: true })

useQuery.ts

import {
  DocumentNode,
  OperationVariables,
  QueryHookOptions as ApolloQueryHookOptions,
  QueryResult,
  TypedDocumentNode,
  useQuery as useApolloQuery,
} from '@apollo/client'
import { useMemo } from 'react'export type QueryHookOptions<TData = any, TVariables = OperationVariables> = {
  suspense?: boolean
} & ApolloQueryHookOptions<TData, TVariables>export const useQuery = <TData = any, TVariables = OperationVariables>(
  query: DocumentNode | TypedDocumentNode<TData, TVariables>,
  options?: QueryHookOptions<TData, TVariables>,
): QueryResult<TData, TVariables> => useSuspenseQuery(useApolloQuery(query, options), options)const useSuspenseQuery = <TData, TVariables>(
  { data, loading, error, observable, ...res }: QueryResult<TData, TVariables>,
  options?: QueryHookOptions<TData, TVariables>,
): QueryResult<TData, TVariables> => {
  const suspense = options?.suspense
  const errorPolicy = options?.errorPolicy || 'none'const promise = useMemo(
    () =>
      suspense &&
      new Promise(resolve => {
        const resolver = () => {
          resolve(true)
          subscription.unsubscribe()
        }
        const subscription = observable.subscribe(({ data, loading }) => {
          if (data && !loading) resolver()
        })
      }),
    [observable, suspense],
  )const proxy = useMemo(
    () =>
      suspense &&
      new Proxy((data || {}) as any, {
        get: (target, prop) => {
          if (!Object.keys(target).length && loading) {
            throw promise
          } else if (errorPolicy === 'none' && error) {
            throw error
          }
          return target[prop as keyof typeof target]
        },
      }),
    [data, error, errorPolicy, loading, promise, suspense],
  )return { data: suspense ? proxy : data, loading, error, observable, ...res }
}

@bignimbus
Copy link
Contributor

Crossposting the latest relevant RFC in the React repo: reactjs/rfcs#229

@jerelmiller
Copy link
Member

jerelmiller commented Oct 25, 2022

Hey all 👋

Just wanted you to be aware that we’ve just opened up an RFC (#10231) where we have a detailed approach to React suspense and SSR. We plan to continue the conversation over there so please have a look! Thanks for all the discussion so far!

@apollographql apollographql locked and limited conversation to collaborators Oct 25, 2022
@jpvajda jpvajda modified the milestones: Release 3.9, Release 3.8 Nov 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.