facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.85k stars 46.51k forks source link

React 18 Suspense unable to track when a promise completes causing SWR failure #23045

Open pkellner opened 2 years ago

pkellner commented 2 years ago

When working with SWR with React 17, you could pass to useSwr on onSuccess parameter which would assign a completion function so that when an async call finished, you could take some action (for example, when not using Relay, you can trigger another component to begin loading data based on the returned data from useSwr.

After this lengthy issue with @promer94, one of the devs on SWR, we found an ugly workaround with useEffect and some negative logic, but it is not satisfactory and will always be hard to reason about as well as do so similar things without introducing bugs. To quote @promer94, i think we need more time to understand concurrent mode before we change current api design. This feels to me like either we don't understand how Suspense works, or there is a serious usability issue if not using Relay.

It's summarized best here: https://github.com/vercel/swr/issues/1733#issuecomment-1001391484 . Assuming this is correct, there is no determinant way to know when components suspense promise completes, and no way to act on that which would be required in multi-component scenarios.

The issue mentioned above https://github.com/vercel/swr/issues/1733 explains in more detail the problem, and the example code that demonstrates it by having to do the ugly workaround for not having onSuccess is in this repo: https://github.com/pkellner/pluralsight-react-18-suspense-swr-problem/blob/master/src/components/CityListItems.js

If you want to see the program working you can view it on this link. Notice that after the city list loads, the first city becomes selected. That was trivial using React 17 with SWR and really hard with React 18 and Suspense enabled.

https://pluralsight-react-18-first-look.peterkellner.net/#

The source for the app with Suspense is here: https://github.com/pkellner/airquality

gaearon commented 2 years ago

We don't recommend libraries to implement Suspense for data fetching yet. (At this point, it's more suited for frameworks because they have full control over when things are called and usually provide a more constrained API.) In particular, a callback like onSuccess doesn't make sense to me in a Suspensey API.

Like you've implemented, an effect is one way to fix the problem. However, fetching something in the child and then setting state on the parent seems like a suspicious pattern by itself (the data flow becomes inside-out). It would help to have a tiny sandbox demonstrating the exact UI pattern you're trying to accomplish that's extracted from the app you posted.

Thanks!

pkellner commented 2 years ago

I think the pattern is not uncommon. I'll think through a sandbox example and add it here.

gaearon commented 2 years ago

Awesome, thanks! It would really help me understand why so I can discuss with the team if there’s a more natural pattern with the same outcome.

pkellner99 commented 2 years ago

FYI, I posted an incorrect example (and deleted it). I'll modify and re-post here.

pkellner commented 2 years ago

(TL;DR)

It seems that using Suspense with a simple data list-detail problem adds significant complexity to the required React app to achieve the same results as without using Suspense.

Here is the before and after code sandboxes for the app built with and without Suspense.

I don't see a straight forward way around having really a really awkward ternary expression on lines 7-11 here as well as all the added complexity around passing around extra props to make this even work. I'd love to know I over-complicated this with Suspense somehow and what that easier way of solving this problem is.

animated-small-architecture

ui

Problem

I've finally got a simple example of what I see as a big problem when building React apps that use Suspense (declarative) vs the imperative way of building React apps. That is imperative meaning taking advantage of useEffect. The main point here is that what seems quite straight forward to me previously, is now much harder (and more complex) to do with Suspense.

What makes things difficult is dealing with displaying the first record of the city list on the right without having to click on that first row. useSwr made it easy with there onSuccess option you assign to your call to useSwr, but if I were to code it with useEffect, it would not be that different. My understanding of Suspense is that it's antithetical to have an option like onSuccess with any Suspense support data library like SWR.

I started a long discussion the the SWR repo with the committers of that rep. Those URLS are URL1 and URL2. There is another similar discussion involving Suspense here: URL3

No Suspense

https://github.com/pkellner/react-no-suspense-issue-23045

codesandbox equivalent to repo

With Suspense

https://github.com/pkellner/react-suspense-promise-issue-23045

codesandbox equivalent to repo

React 17, No Suspense Architecture

react17-no-suspense

Without using Suspense, to implement a list and detail React app that shows the detail of the first row after the list is renders is very simple. It just requires two components with nested under an main component that each has their own async data calls. These components share common state (selectedCityId, setSelectedCityid) between them.

On render completion in the CityList component, setSelectedCityId is called causing the CityDetail component to re-render, and its useEffect (implemented with onSuccess with useSwr) causes the detail data to be retrieved (without the user having to click on the first city in the list).

Here is the simple code in the CityList component that includes passed in props and display logic.

export default function CityDetail({ selectedCityId }) {
  const url = selectedCityId
    ? `${restBase}/api/city/${selectedCityId}`
    : "noSelectedCityId";

React 18, With Suspense Architecture

react18-with-suspense-showing-boundaries

For Suspense to work, my thinking is that I needed to move the CityDetail component inside the CityList component so that CityDetail would have access to the CityList data, as well as the selectedCityId. That way, the CityDetail component could check to see if the CityList data is loaded, and then, and only then show the appropriate city.

What makes this tricky is that now, CityDetail has multiple concerns. It must worry about two cases.

In my solution, the complexity is increased in the following ways. It means passing the selectedCityId down two levels in the component hierarchy as well as now, having to pass the cityList data itself down one level to the cityDetail component. It also causes a complex ternary expression. Here is that expression and received props in CityDetail which bothers me as it's complex to get right and of course error prone.

export default function CityDetail({ selectedCityId, isPending, cities }) {
  const selectedCityIdLocal = selectedCityId
    ? selectedCityId
    : cities && !selectedCityId
    ? cities[0].id
    : undefined;

  const url = `${restBase}/api/city/${selectedCityIdLocal}`;

Side Notes

I have tried the solution where I add useEffect to the detail component where the side effect is setting the selectedCityId. My concern there is that I don't believe that the useEffect function call will happen after the Suspense promises complete. Is that the case? I posted a question on StackOverflow hoping to get some clarity there, but as of now, there are no responses. I don't know of any other place to look for an answer to that question.

Also, as discussed in this issue, including a component that includes Suspense, causes NextJS to not process button click events correctly. I don't know if that is a NextJS, SWR or React Suspense issue.

My bigger question though is that as now, React 18 and Suspense are released, I'm working on a Suspense course at Pluralsight where I get hundreds of learners a day watching my courses and I'm uncomfortable using SWR as that team claims that their Suspense implementation is still experimental.

I don't know of any data libraries that are not experimental that I should include in my upcoming Suspense course. Please let me know of any such library (besides Relay, which requires GraphQL which is beyond the scope of my course).

gaearon commented 2 years ago

I'll try to write a longer answer later.

The short answer is I would not recommend including a library like useSWR with its Suspense mode in a course at this time. As noted in the release post, we only support opinionated frameworks at this point rather than ad-hoc component-level data fetching. Relay is the one we're aware of that works well. We anticipate that Next.js will add support for using Suspense with getServerSideProps and such in the future, and we'll support that when it's out. So maybe it's good to wait until that. We're not sure whether ad-hoc helpers like useSWR will ever be recommended to use with Suspense, so that's something we need to come back to later.

pkellner commented 2 years ago

I'm holding off on courses for now that include Suspense and data, based on my understanding and comments here from @gaearon .

Pluralsight has asked me to both create a "What's New in React 18 Course", as well as create a blog post explaining what is going on with React 18, Suspense and Data at the moment. Most of Pluralsight's customers are enterprise and for the most part will not be using Relay

I've created the post that includes a discussion of "The Elephant in the Room" and I would really like to confirm that I've not gotten anything drastically wrong. Here is the preliminary URL. Ultimately, it will go to the Pluralsight blog similar to one I did before where the distribution is quite wide.

https://github.com/pkellner/pluralsight-suspense-blog-post

That's topic 1. Second topic is I plan on creating an example similar to the one Dan did a long time ago using a FakeApi. I plan on being very very clear that this code is not to be copied and pasted into production. I also may include a relay example that does the same thing but with very little programming instruction. The course will likely be in total about an hour so I don't have time to really teach Relay and all that goes into it (specifically, GraphQL).

I have most of the source code now built I plan on teaching from in my, soon to be released, "What's New in React 18" course. Based on @gaearon 's feedback from StackOverflow, I've taken care of the initial issue that I was having with onSuccess not being implement as part of SWR suspense mode. Now I realize that useSwr is something that should not be used to explain Suspense so I promise not to ask any more questions about that until Suspense and Data is more fleshed out.

Here is a link to my Code I plan to teach from using something similar to Dan's FakeAPI, and the associated CodeSandBox. I would love feedback on this code, with the complete understanding it is not production ready code.

https://github.com/pkellner/pluralsight-react-18-whats-new-prelim

https://codesandbox.io/s/github/pkellner/pluralsight-react-18-whats-new-prelim

gaearon commented 2 years ago

One thing I want to flag early is that we've basically dropped the term "concurrent mode" completely because there is no "mode" now. It's just a set of features which are opt-in. See https://github.com/reactwg/react-18/discussions/64 for historical context. Our "modern" messaging is what you can see in the release blog posts (https://reactjs.org/blog/2022/03/29/react-v18.html) and ReactConf keynote (https://reactjs.org/blog/2021/12/17/react-conf-2021-recap.html#react-18-and-concurrent-features).

gaearon commented 2 years ago

As for the framing of the post, I think the important note to convey is that Suspense will likely come to frameworks first. Relay is one, but you will likely see Suspense support in Next.js/Remix/Gatsby etc before you see it in “libraries” like useSWR etc.

pkellner commented 1 year ago

related. Maybe this will solve it? https://github.com/reactjs/rfcs/pull/229