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

Suspense and Concurrent Mode in ReSift #32

Closed
ricokahler opened this issue Oct 26, 2019 · 9 comments
Closed

Suspense and Concurrent Mode in ReSift #32

ricokahler opened this issue Oct 26, 2019 · 9 comments

Comments

@ricokahler
Copy link
Contributor

ricokahler commented Oct 26, 2019

The React team has finally released docs for suspense for data fetching and concurrent mode 🎉. They've clarified a bit of how it works and how its intended to be used, and my first impression is that it can actually fit within our current APIs nicely without any big breaking changes.

Reading their docs is a prereq to this issue. and so is watching this talk from the Relay team.

They say the correct way to use suspense is to "render as you fetch", meaning that you reveal parts of the API that make sense to reveal if you have enough data to reveal it. Here is a direct quote from their new docs:

Although it’s technically doable, Suspense is not currently intended as a way to start fetching data when a component renders. Rather, it lets components express that they’re “waiting” for data that is already being fetched.

which sums up the idea of "render as you fetch".


What does this mean for ReSift?

Well, fortunately, not that much. Many simple fetching libraries rely on "fetch-on-render" techniques because the state of the fetches inflight live in the component. Fortunately for ReSift, the state of our fetches lives in global state, and we've already discovered that we need to split up fetching (e.g. dispatch(getPerson())) from getting the data (e.g. useData(getPerson)).

This means that we can advise users to initiate fetches for components at some higher component level to abide by the "already being fetched"/"fetch-as-you-render" philosophy. This is possible because our useDispatch and useData APIs are separate.

Initial thoughts on Suspense and Concurrent Mode in ReSift (revised)

Here's how I think it should work (disclaimer: i have not tested to see if this will work):

  1. useData should suspend by default (i.e. throw a promise) and we should add a configuration object with the flag noSuspend (or maybe allowNull or similar) to allow the hook to return null instead of causing suspense (e.g. useData(getPerson, { noSuspend: true })).
  2. we advise our users to dispatch higher up in the tree to prevent "fetch on render" patterns*
  3. we make dispatch work with startTransition of useTransition

I still have to test whether this approach makes sense but there's my initial thoughts on how that should work.

Adoption strategy/road map concerns

Suspense + concurrent mode should stay in an experimental as long as React has them in an experimental channel. See #37 for more details.

@ricokahler ricokahler pinned this issue Oct 26, 2019
@ricokahler
Copy link
Contributor Author

ricokahler commented Oct 29, 2019

I have some new news about supporting suspense and concurrent mode: https://stackoverflow.com/questions/58611408/what-kinds-of-state-updates-can-we-defer-with-usetransition

TL;DR, we have to remove redux and replace it with React context. I don't think this will be too difficult but it's something to do.

image

@ricokahler
Copy link
Contributor Author

I have a CodeSandbox with working but naive implementation of Suspense. This demo uses <SuspenseList> orchestrate the loading of images using a new custom data service for fetching images.

https://codesandbox.io/s/resift-rentals-with-suspense-nnzfz

It works by throwing a promise that subscribes to the redux store. It re-runs the selector until the data is present. After that, it resolves the promise and tells suspense that we can render.

This works but it's problematic bc (from the post above), redux state is incompatible with useTransition

I'll report back with more experiments!

@dai-shi
Copy link

dai-shi commented Nov 8, 2019

can I wrap an update to Redux's dispatch in startTransition?

No because Redux (currently) has its own state that's not managed by React. If Redux was using React state as the source of truth, it would have been possible.

You might want to try react-tracked.
It is using React state as the source of truth, and it has RR compatible useSelector.
I'm not suggesting to replace with this (because the power of react-tracked is useTrackedState),
but you should be able to try it easily. (and I'm interested in hearing the result.)

This was referenced Nov 9, 2019
@ricokahler
Copy link
Contributor Author

ricokahler commented Nov 10, 2019

Alright I'm back with some experiments and I think I have more direction on how concurrent mode should work in ReSift.

First I want to establish some goals and better define what "supporting suspense and concurrent mode" means for ReSift 👇

The goal should be to support suspense and concurrent mode as much as Relay does. This means to utilize the React APIs Relay utilizes. The objective is then to have the same UX benefits as Relay.

I've been reading their experimental docs, and it's some pretty dense stuff 🤯


To me, supporting suspense and concurrent mode for ReSift means establishing a framework of using:

  • Suspense and SuspenseList
  • useTransition and useDeferredValue

Out of the suspense supporting data fetching libs, I have yet to see one support useTransition or even mention SuspenseList so really the only reference implementation that does all of this is Relay.

Here are the two libs I've been keeping an eye on (let me know if there are more):

I also checked to see if the apollo client is working on anything and I didn't see nothin.


Supporting <Suspense> and <SuspenseList>

Using the other libraries that have come out as an indicator, it seems like supporting the suspense component is the easiest to achieve.

In order to make suspense work, we have to throw a promise that will resolve when the data is ready. The question is now "what to throw?"

A working but probably naive implementation is to throw a promise that subscribes to the redux store and resolves when the selector returns a status that is normal. See here for a demo.

This demo is interesting because it uses suspense to load images. I think it's a somewhat accurate real-world scenario/want for suspense. In the demo, there are ~20 rows, each row loads 20 images = nearly 400 images. 400 images that could possibly suspend at once.

This is probably a bit of an extreme use-case but it's something that could happen somewhat often. My impression from the react conf talks is that image loading with suspense is a very common use case.

Given that scenario, I can foresee two possible issues with this approach:

  1. Under the current implementation, the useStatus selector could be too slow. Right now it doesn't do any memoizing or re-selecting inside of the store so having many many subscribers (e.g. one subscription per image) would probably negatively impact performance.
  2. Redux's current subscription mechanism uses an array. When removing a subscription, it uses splice which, I think costs O(n). When we have 400 images constantly subscribing and unsubscribing, that could result in O(n^2) type of performance? I'm not entirely sure if we need to be concerned about this but it could cause slow downs similar to the Search Results refresh project (Sift internal project) if not careful.

For now, I'm gonna assume that subscribing to some mechanism that emits whenever any change to the store happens and re-running all the relevant selectors from suspending components is efficient enough. This may continue to be redux or maybe it'll be our own implementation.

Minus the performance concerns, this approach is appealing because it answer the question "what to throw" in a simple way that guarantees when the promise resolves, the data is ready. I'm open to suggestions for "what to throw" but for now I think this will work.

Supporting useTransition and useDeferredValue

This section is essentially "supporting concurrent mode". This is one is actually quite a bit harder to get right IMO. This involves supporting useTransition and useDeferredValue correctly.

From my initial experiments, here is what I've learned:

  1. useTransition will (afaik) "freeze" the state you're transitioning. So if I wrap any react-trackable setState with startTransition, any component that is reading that state will get the version frozen in time from when you called startTransition. This includes state that is or is not within a suspense boundary. This is incorrect. See below for more details.
    1. See this demo. It has global state in context and some transitions. When you type in the input, it updates the state, but when you suspend then type, the state you see is frozen until it's no longer pending.
  2. useTransition will end its transition when either:
    1. the timeout happens
    2. the related suspending components have stopped suspending
  3. useTransition only works on react-trackable state. it doesn't work with redux's dispatch (at the moment but this could change).
    1. See @eddyw's demo to observe that dispatch doesn't work with useTransition from Concurrent Mode reduxjs/react-redux#1351.

So given that information here's some takeaways:

  1. We need to reevaluate our state store shape. useTransition freezes the whole state you're trying to transition. I'm not sure it makes sense to freeze all components that use ReSift during a transition. We might need to break it up into smaller parts.
    1. To clarify (afaik), when React "freezes" state, the state will still receive updates in the background but React will simply show the state at the point of the startTransition call. useTransition does not block it from getting updates.
  2. useTransition doesn't work with redux's dispatch so we'll have to either wait for them to switch to a compatible version or switch to a different state store (like @dai-shi's react-tracked etc) or roll own, however, for the reasoning above, we might not want a big single store.

Here's what I think we should do:

  1. Figure out the correct state/store shape that works with useTransition given that we know the state we transition will freeze across the whole app.
  2. We should consider the question "what to throw" as in what to throw to tell react data is present.
  3. We should ensure the state we choose is compatible with concurrent mode. Currently my definition for "compatible" means no tearing + something that works with useTransition.

Some quick thoughts:

  1. We should still manage the store state outside of React so we can throw something that subscribes to this store and resolves when the data is present from anywhere.
  2. We should copy this store state into React state via context or similar. I'm thinking maybe @dai-shi 's use-context-selector or maybe his react-tracked.

This answers "what to throw" and "CM compatible" but it doesn't really answer how to split up state so react can freeze it with good UX.

That's all I got for now!

@pearlzhuzeng
Copy link
Member

Hey @ricokahler, your comment is packed with good thoughts! I feel that I need to read more into each linked article to offer insights. For now, I have a very general question: By 'supporting suspense and concurrent mode', do you just mean that ReSift will function as expected when users have concurrent mode enabled, or do you mean re-writing part of ReSift to use suspense, or, do you mean adding new ReSift apis that have concurrent mode built in so ReSift users can get the benefits without having to write code with suspense and other concurrent mode hooks themselves?

@ricokahler
Copy link
Contributor Author

ricokahler commented Nov 12, 2019

By 'supporting suspense and concurrent mode', do you just mean that ReSift will function as expected when users have concurrent mode enabled, or do you mean re-writing part of ReSift to use suspense, or, do you mean adding new ReSift apis that have concurrent mode built in so ReSift users can get the benefits without having to write code with suspense and other concurrent mode hooks themselves?

@pearlzhuzeng I mean a combination of all three in some ways 😅. It's interesting.

The high-level goal I outlined above is more or less to have ReSift have the same UX benefits as Relay. This means defining some framework that uses Suspense and Concurrent Mode to get those UX benefits.

Lower level this means (afaik):

  1. Using a state that is compatible with concurrent mode. From this SO question Dan answered, it seems like that means using something like React Context to hold state.
  2. Figuring out what promise to throw. throwing a promise is how to notify React that your component is ready to retry rendering. This promise should resolve when the data is ready.
  3. Possibly re-architecting our state shape to work well with transitioning state. By state shape, I mean the shape of the state in the ReSift store (like a shape that could be described in a typescript interface). If you read above from my experiments, it seems like React will freeze whatever state you're transitioning across the whole app (so that it's consistent). What I'm wondering here is if it makes sense to freeze the entire ReSift state during a transition. This is more of a product question/UX concern vs a technical question. It might be fine to freeze the whole ReSift state during a transition or it might be confusing to the end-user. I don't know yet.

So to answer your questions more directly:

do you just mean that ReSift will function as expected when users have concurrent mode enabled

Yes but there's more to it. Because our goal is to have the same UX benefits as Relay, we have to create some framework to not only works with concurrent mode enabled but has extra UX benefits when using this framework with concurrent mode.

do you mean re-writing part of ReSift to use suspense

I think supporting suspense and concurrent mode does involve re-writing parts of ReSift but I don't think ReSift's internal APIs will use Suspense or useTransition (or similar). Instead we'll ensure that ReSift will works along with Suspense, SuspenseList, useTransition, etc, so when ReSift users want to get the benefits of Suspense, they'll have to use the <Suspense> component (and similar) themselves but ReSift will be designed to work with those.

do you mean adding new ReSift apis that have concurrent mode built in so ReSift users can get the benefits without having to write code with suspense and other concurrent mode hooks themselves?

From my answer above, I think the right approach is to make ReSift work with the official React APIs. Right now, I don't think this means adding any new APIs or using the React API internally in ReSift. I think our current APIs will actually work very nicely with suspense and concurrent mode.

For example, I'm envisioning this is how suspense + concurrent mode + resift code could look like:

import React, { Suspense, useTransition, lazy } from 'react';
import { useDispatch, useData } from 'resift';
import makeGetPost from './makeGetPost';
const PostContent = lazy(() => import('./PostContent'));

const useStyles = /* ... */;

function Post({ id }) {
  const classes = useStyles();
  const dispatch = useDispatch();
  const [startTransition, isPending] = useTransition({ timeoutMs: 3000 });
  const getPost = makeGetPost(id);

  useEffect(() => {
    startTransition(() => {
      dispatch(getPost());
    });
  }, [getPost]);

  return (
    <div>
      <h1>Post:</h1>

      <Suspense fallback={<SkeltonPost />}>
        <PostContent
          fetch={getPost}
          className={classNames({
            [classes.transitioning]: isPending,
          })}
        />
      </Suspense>
    </div>;
  );
}

export default Post;
import React from 'react';
import { useData } from 'resift';

function PostContent({ fetch }) {
  const post = useData(fetch);

  return (
    <div>
      <h2>{post.title}</h2>
      {/* ... */}
    </div>
  );
}

export default PostContent;

@ricokahler
Copy link
Contributor Author

ricokahler commented Nov 12, 2019

I tweeted Dan Abramov and Andrew Clark with some suspense questions and my initial thought of "react freezes the state you're transitioning" is completely wrong lol.

I don't think we need to re-architect our state shape to consider a react "freezing" state because that's incorrect. Instead, now I'm thinking we need to get ReSift state into a react-manage state and then solve the "what to throw" problem.

@dai-shi
Copy link

dai-shi commented Jan 26, 2020

Hi, that's what we call "state branching". It's not official words.
Just yesterday, I re-implemented one of my global state libs, and make it support state branching (without react contexts, which I've done for other libs).
It's an opt-in feature, that "links" its external state to the "react-manage state".
It's not documented, but you can see the code if you are adventurous.
https://github.com/dai-shi/react-hooks-global-state/blob/53560e48518c4c5a3e03bcb2476ed8a9ed391740/src/createContainer.ts#L98-L116
(Hmm, I found it not very readable.)

@ricokahler
Copy link
Contributor Author

@dai-shi oh that’s fascinating. thanks for sharing!

@ricokahler ricokahler unpinned this issue Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants