Suspense, and how to name things.

[aulneau]

Continued from https://github.com/blockstack/stacks-wallet-web/pull/1326/files/922891daedaea060bd3d603d25e2e29f12f8664f#r655165139

With the refactor away from recoil over to jotai, we now are using Suspense in a much more direct and impactful way. We previously had a few instances where async atoms were used in components that required being wrapped in a Suspense boundary, but recoil had the Loadable concept which converts a suspense atom to one that wouldn't suspend. Jotai does not have this same concept.

Before digging in, I'd highly recommend reading these so we are all at a similar place when it comes to context around what suspense/concurrent mode is, and how it's used related to data fetching.

https://reactjs.org/docs/concurrent-mode-suspense.html
https://reactjs.org/docs/concurrent-mode-patterns.html
reactwg/react-18#61 (read as many of these discussions as you can, very helpful!)

During my refactor PR, I ran into the situation where we have components that are async, require a fallback, and could be wrapped in an error boundary. Take this component as an example:

// this component is what suspends
// we CANNOT place this anywhere without being aware of the closest Suspense boundary
// I view this more as a private function for the component
const SendButtonSuspense = () => {
    const assets = useTransferableAssets(); // an async atom
    const isDisabled = !assets || assets?.length === 0;
    return <TxButton isDisabled={isDisabled} path={ScreenPaths.POPUP_SEND} kind="send"/>;
};

// we want to show the same button, but disabled while it loads data
// this could be exported, but why?
const SendButtonFallback = memo(() => (
    <TxButton isDisabled path={ScreenPaths.POPUP_SEND} kind="send"/>
));

// we can place this component anywhere, because it's self contained
// the main export of a "suspense" component
export const SendButton = () => (
    <React.Suspense fallback={<SendButtonFallback/>}>
        <SendButtonSuspense/>
    </React.Suspense>
);

To me, for any given "async" component, there will be a few different parts:

• the main component which is async and suspends
• an optional fallback component (if we want this component to always have a specific fallback, sometimes we want to defer higher up)
• the encapsulated wrapper which contains: the Suspense boundary, the fallback, and the suspending component
• an optional error boundary and error fallback

The reason I defaulted to naming the suspending component with a suffix of Suspense is because I think it's important to know by seeing the component name that it will suspend, where if we use SendButton, a developer can be free to place it anywhere.

Here is what @kyranjamie suggested:

Here's an attempt: I'm not 100% on these, there may be better alternatives, but they address the above in not making reference to the containing hook's implementation:

<SendButton /> --> base component describing what the component is, dumb
<SendAction /> --> component describing what the component does, higher level (doesn't reference specific UI element used), smart, contains logic
<SendActionWithFallback /> --> component responsible for describing suspense and fallback case 🤷🏼‍♂️

I wonder if there's an argument for whether we should create separate components with the suspense in. In this instance for example, couldn't it be considered the role of HomeActions to provide a fallback a the given component? 🤔

The other thing to consider is it will be more and more common for most of our components to be async as time goes on, and I wonder if we need to even consider this kind of naming convention at all. FB has been using these patterns for a long time, and there is never anything done like this. (They just name them what they are, and wrap them as needed)

What does everyone think?

[kyranjamie]

Thanks for starting this discussion Thomas. Made it through the Concurrent Mode Patterns article. Big read.

My impression is that we'd best off completely avoiding creating components just to wrap the <Suspense /> boundary.
Instead describing the Suspense, and fallback (and/or error boundary), in the page, or feature-level component (i.e, generally not having components with a Suspense root).

• Tertiary level component structuring is complex: Simply the fact we're having this discussion kinda highlights some of the difficulty. Both @aulneau and I have attempted it and I don't think either of us have come to particularly clean solution.
• Patterns will emerge: This is new tech. It's not even officially supported in many cases. We'll see if patterns emerge later and can easily adapt.
• No immediate need to decouple: While it looks "cleaner" to have a feature exposed as only a single component, there isn't a real need to decouple imo. What do we gain? I think we might even lose some flexibility, such as if we need to use the same component with a different fallback elsewhere.
  As long as the upper components don't become polluted with async logic for the sake of its children—which with this pattern they wouldn't—lets just keep the suspense boundary inline where the feature is used. You could even argue its the role of a route-level/feature component is to compose the states that make up the page/layout.
• Composing suspends: Many of the documentation examples solve race conditions and rendering-order issues by moving around the placement of the suspense/error boundaries. By creating "suspense-only" components, we'll be making this composition harder/impossible. Consider if some change resulted in the <ReceiveButton /> suspending, and the designs used a single fallback component for both buttons, rather than an individual button fallback. We'd still want separate components for each button, but the Suspense boundary would have to be wrapping both of them.
• Avoids assumptions: Putting the suspense in a component makes two assumptions:
  1. it will always suspend by itself
  2. it will always show fallback <X />

Further, its inline with some of the docs examples, like:

function ProfilePage({ resource }) {
  return (
    <>
      <ProfileDetails resource={resource} />
      <Suspense fallback={<h2>Loading posts...</h2>}>
        <ProfileTimeline resource={resource} />
      </Suspense>
      <Suspense fallback={<h2>Loading fun facts...</h2>}>
        <ProfileTrivia resource={resource} />
      </Suspense>
    </>
  );
}

Per the example above, I'd suggest simply removing the component completely:

const SendButtonFallback = memo(() => (
  <TxButton isDisabled path={ScreenPaths.POPUP_SEND} kind="send"/>
));

const SendButton = () => {
  const assets = useTransferableAssets(); // an async atom
  const isDisabled = !assets || assets?.length === 0;
  return <TxButton isDisabled={isDisabled} path={ScreenPaths.POPUP_SEND} kind="send"/>;
};

export const HomeActions = props => {
  return (
    <Stack>
      <React.Suspense fallback={<SendButtonFallback />}>
        <SendButton />
      </React.Suspense>
      <TxButton path={ScreenPaths.POPUP_RECEIVE} kind="receive" />
    </Stack>
  );
};

By omitting the component that only handles the suspense boundary, we solve this naming paradox completely. Plus, in none of the react doc examples do they declare a component suspends by naming it with a suffix. So there's no precedent to do this, and I think this is intentional. A component doesn't need to expose its implementation details (that it suspends).

My point boils down to: I think we should postpone adding this abstraction. Perhaps we do need it. Maybe we don't. But it's easier to add an abstraction later, than remove it later, imo.

[aulneau]

My point boils down to: I think we should postpone adding this abstraction. Perhaps we do need it. Maybe we don't. But it's easier to add an abstraction later, than remove it later, imo.

I think this makes a lot of sense, and agree with you!

[andresgalante]

As far as I know, Suspense is an experimental feature, not supported by React yet and in a version that we don't use.

I'd rather park this discussion until we see a need for performance improvements that suspense would help with. What do you think?

[aulneau]

As far as I know, Suspense is an experimental feature, not supported by React yet and in a version that we don't use.

I'd rather park this discussion until we see a need for performance improvements that suspense would help with. What do you think?

This is not accurate -- we've been using Suspense (albeit sparsely) since the early versions of our extension.