-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Comments
@hwillson I am slightly confused-is react supporting suspense for data fetching? It's not mentioned anywhere. They only explicitly list these three usecases:
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 thanks that explains it well enough. |
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 |
@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. |
@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) |
@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 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. |
@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? |
@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 My backup would have been |
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. |
@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 😅 |
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. |
@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. |
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. |
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 |
@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) |
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) |
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! |
@NTag Thanks for checking in on this feature. We have moved it to our |
Wrote this today, surprisingly it works as a drop in replacement to
Implementationimport { 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 };
}; Usagefunction Example() {
<React.Suspense fallback={'My Fallback'}>
<MyComponent />
</React.Suspense>
}
function MyComponent() {
const { data } = useSuspenseQuery(MyQuery);
return <>{data.someValue}</>
} |
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 🤓. |
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. |
@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? |
@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 |
@pkellner as I said to you previously, Apollo is not an "ad hoc" library but a full fledged client like Relay. Like dan said:
Anyways, I don't see the point of tagging people and turn down alternatives published by people. |
@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. |
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. |
@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. |
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. |
@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: If you look at the console you'll see that |
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. |
@jamesmfriedman Can I get some clarification what the use flow is with Suspense(hacks or when Apollo 3.9 comes out). |
Based on @jamesmfriedman solution, one could make a const { data, error } = useQuery(query, { suspense: true })
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 }
} |
Crossposting the latest relevant RFC in the React repo: reactjs/rfcs#229 |
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! |
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).The text was updated successfully, but these errors were encountered: