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

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

Open
pkellner opened this issue Dec 29, 2021 · 10 comments
Open
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion

Comments

@pkellner
Copy link

pkellner commented Dec 29, 2021

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: vercel/swr#1733 (comment) . 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 vercel/swr#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

@pkellner pkellner added React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion labels Dec 29, 2021
@gaearon
Copy link
Collaborator

gaearon commented Mar 30, 2022

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
Copy link
Author

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

@gaearon
Copy link
Collaborator

gaearon commented Mar 30, 2022

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
Copy link

pkellner99 commented Mar 30, 2022

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

@pkellner
Copy link
Author

(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.

  • After the apps intial renders, the first entry (or row) in the CityList's component should be asynchronously retrieved and shown in the detail component
  • When any row in the CityList is clicked, the detail component should asynchronously retrieve and show the data in the detail component.

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
Copy link
Collaborator

gaearon commented Apr 11, 2022

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
Copy link
Author

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
Copy link
Collaborator

gaearon commented Apr 20, 2022

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 reactwg/react-18#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
Copy link
Collaborator

gaearon commented Apr 20, 2022

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
Copy link
Author

related. Maybe this will solve it? reactjs/rfcs#229

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion
Projects
None yet
Development

No branches or pull requests

3 participants