[WIP] Supporting Suspense

[jmeistrich]

This discussion originally started by @texastoland in https://github.com/LegendApp/legend-state/discussions/125#discussioncomment-6546293

I've made a simple hook that I think might be all we need to support Suspense? I have not actually used Suspense before, though if it's this easy I may start using it more 😂. So would any of you Suspense wizards out there be able to give this a test and let me know if it works correctly? If so I'll add it to the repo in an update.

import { isPromise, Selector } from '@legendapp/state';
import { useSelector } from '@legendapp/state/react';

export function useSelectorSuspense<T>(selector: Selector<T>): T {
    const value = useSelector(selector) as any;
    if (isPromise(value)) {
        throw value;
    } else if (value?.error) {
        throw value.error;
    } else {
        return value;
    }
}

And this is my simple test case:

const state$ = observable(
    new Promise<string>((resolve) => {
        setTimeout(() => {
            resolve('hello');
        }, 1000);
    })
);

function Test() {
    const value = useSelectorSuspense(state$);
    console.log({ value });
    return <div>{value}</div>;
}

export default function App() {
    return (
        <div>
            <div>Suspense test</div>
            <Suspense fallback={<div>Loading...</div>}>
                <Test />
            </Suspense>
        </div>
    );
}

[jmeistrich]

Also if this does work, what would be your preferred way of using it?

• Is this a good name for a hook?
• It could be a new function added to observables a la use() named useSuspense()?
• Or it could even be done in a parameter to use() like state.use({ suspense: true })?

[texastoland]

Throwing promises is deprecated. I moved my original explanation here from from https://github.com/LegendApp/legend-state/discussions/125#discussioncomment-6546293. Happy to clarify anything at all 🙊

I reread the use RFC a few times: https://github.com/acdlite/rfcs/blob/first-class-promises/text/0000-first-class-support-for-promises.md

Takeaway 1

in the current version of React, an unstable mechanism allows arbitrary functions to suspend during render by throwing a promise. We will be removing this in future releases in favor of use.

I couldn't find any issues or discussions in TanStack Query so React will probably wait on deprecation but it's worth adding a TODO to fix it here too:

https://github.com/LegendApp/legend-state/blob/5e71281e19779c857c562d767c0c5a77ac3fe8ee/src/react-hooks/useObservableQuery.ts#L130-L133

Takeaway 2

the promise passed to use may or may not be the same promise instance that was passed during the last attempt. However, regardless of whether the instances are identical, we can assume that they represent the same result, as long as none of the props or state have changed in the meantime. React will reuse the result from the previous attempt, and ignore the promise that was created during the replay.

TLDR: use doesn't need a reference to the original promise after all 😳

Takeaway 3

If props or state have changed, ... React will try is to check if the promise was read previously ... by adding additional properties to the promise object:

• The status field is set to one of "pending", "fulfilled", or "rejected".
• When a promise is fulfilled, its value field is set to the fulfilled value.
• When a promise is rejected, its reason field is set to the rejection reason (which in practice is usually an error object).

TLDR: the interface above bypasses then in re-renders and provides DX like most query libraries (compared to current behavior):

type PromiseResult<Value> = 
  | { status: 'pending' }
  | { status: 'fulfilled', value: Value }
  | { status: 'rejected', reason: unknown }

Validating assusmptions

• https://github.com/facebook/react/blob/fdb368d9e7430b50b9c506e76ccd46bf0576ef9b/packages/react-reconciler/src/ReactFiberHooks.js#L1084-L1087
• https://github.com/facebook/react/blob/fdb368d9e7430b50b9c506e76ccd46bf0576ef9b/packages/react-reconciler/src/ReactFiberHooks.js#L1061
• https://github.com/facebook/react/blob/fdb368d9e7430b50b9c506e76ccd46bf0576ef9b/packages/react-reconciler/src/ReactFiberThenable.js#L73-L99

Proposal

• Fullfilled promises should continue to be .set()/replaced
• Promise results should be then-able
• Promise results should satisy React's interface inspired by Promise.allSettled
• Success objects should resemble:

  {
      status: "fulfilled",
      value,
      then(resolve) { resolve(this.value) }
  }

• Failure objects should resemble:

  {
      status: "rejected",
      reason: error,
      then(_, reject) { reject(this.reason) }
  }

• useSelector should delegate then-able nodes to React.use() if Suspense is enabled in Legend

The changes would be:

1. .set() the proposed objects instead of value and { error }: https://github.com/LegendApp/legend-state/blob/d28b3777e4314f1749d49f4a13ac7750154ea6cb/src/ObservableObject.ts#L506-L508
2. Use use in useSelector: https://github.com/LegendApp/legend-state/blob/main/src/react/useSelector.ts
3. Make it configurable so non-Suspense components don't throw errors

[jmeistrich]

@texastoland It looks like the underlying implementation of use just throws a Promise: https://github.com/facebook/react/blob/fdb368d9e7430b50b9c506e76ccd46bf0576ef9b/packages/react-reconciler/src/ReactFiberThenable.js#L180

But I see from the Valtio thread that React will force using use, though I'd love to know where they said that if you know? The information around Suspense seems to be all over the place... Is it in that RFC and I didn't see it?

Do you think it would work to keep the implementation in the original post but change throw value to use(value)? So it would just return the value normally unless it's a promise in which case it would forward it to use?

Do you know if there is a shim to make use backwards compatible to React 17? If not, maybe we want to call use if it's available, otherwise throw the promise to work in 17?

[texastoland]

Here's the RFC again: https://github.com/acdlite/rfcs/blob/first-class-promises/text/0000-first-class-support-for-promises.md

It looks like the underlying implementation of use just throws a Promise

Correct. That's why Suspense must be opt-in. Otherwise non-Suspense components will error out. See Resuming a suspended component by replaying its execution:

RFC: If a promise passed to use hasn't finished loading, use suspends the component's execution by throwing an exception.

But see also Enable compiler-driven optimizations:

RFC: only Hooks (in contrast to promise throwing) will be allowed to suspend. An auto-memoizing compiler (codename Forget) can take advantage of this knowledge to prevent arbitrary function calls from unnecessarily invalidating a memoized computation.

React will force using use, though I'd love to know where they said that if you know?

See my Takeaway 1 from the same paragraph as previous:

RFC: in the current version of React, an unstable mechanism allows arbitrary functions to suspend during render by throwing a promise. We will be removing this in future releases in favor of use.

maybe we want to call use if it's available, otherwise throw the promise to work in 17?

Possibly! Just a question if you want backwards compatibility for an unstable API?

Do you think it would work to keep the implementation in the original post but change throw value to use(value)?

IIUC you want to refactor promise handling out of createObservable/set into useSelector right? But that wouldn't be nice DX for non-Suspense anymore 🙀 See examples further down.

IMO useSelector/property.use should delegate directly to React.use instead of another function. It would receive an options argument like { suspend: true } which could be configured globally for convenience.

   } else if (value?.error) {
       throw value.error;

Also I can't think of a case for rethrowing error values?

Better to step back and look at the 3 cases to support:

Option 1

1. useObservable (current behavior):

   const result = state.promise
   
   // these aren't very intuitive
   const isPending = isPromise(result)
   const isRejected = "error" in result
   const isFulfilled = !isPending && !isRejected
   
   if (isPending) {
     // TypeScript understands result is a Promise
   } else if (isRejected) {
     // TypeScript understands result has an error
   } else { // if isFulfilled
     // TypeScript doesn't understand result's type but is fixable
   }

2. useSelector without Suspense (current behavior): same as above except state.promise.use()

3. useSelector with Suspense (your proposal):

   const data = state.promise.use({ suspend: true })
   // pending would suspend
   // rejected would propogate to an error boundary
   // TypeScript could understand data's type

I like the parity between Case 1 and 3 but they can't currently coexist. Case 1 relies on overwriting the promise with its result. Case 3 relies on passing a then-able object to use. It could work if proxies created ObservablePromises that store their promise but return their result. This would also fix nested promises (state.api.promise) not evaluating like root promises.

Option 2

1. useObservable (my proposal):

   const { status, value: data, reason: error } = state.promise
   if (status === "pending") {
     // TypeScript understands data and error are undefined
   } else if (status === "rejected") {
     // TypeScript understands error exists
   } else { // if "fulfilled"
     // TypeScript understands data's type
   }

2. useSelector without Suspense (current behavior): same as above except state.promise.use()

3. useSelector with Suspense (my proposal):

   const data = state.promise.use({ suspend: true })
   // identical usage as Option 1
   // or state.promise.use()
   // if { suspend: true } confgured globally

This lacks the parity of your API but is nicer to use without Suspense. It also enables storing POJOs internally instead of promises if that's beneficial.

Happy to throw together a POC of either alterntive. PS I appreciate the collaboration 🙏🏼

[jmeistrich]

My proposal is simply to add support for Suspend to useSelector. I had experimented with it as a separate hook, but I believe this should be all the changes needed, adding the 7 lines between the comments:

export function useSelector<T>(selector: Selector<T>, options?: { suspend: boolean }): T {
    if (tracking.inRender) {
        return computeSelector(selector);
    }

    const ref = useRef<SelectorFunctions<T>>();
    if (!ref.current) {
        ref.current = createSelectorFunctions<T>();
    }
    const { subscribe, getVersion, run } = ref.current;

    const value = run(selector) as any;

    useSyncExternalStore(subscribe, getVersion, getVersion);

    // Suspend support
    if (options?.suspend) {
        if (isPromise(value)) {
            throw value;
        } else if (value?.error) {
            throw value.error;
        }
    }
    // / Suspend support

    return value;
}

The RFC says "this means only Hooks will be allowed to suspend" which I think means we can throw in our useSelector hook and that should be fine. But also it suggests using use so I'm not quite sure exactly what it means. I'll reach out to my friend on the React team and ask him. For now I think we should just throw the promise and not worry about use because that seems to be work and be well-supported, until there is a more stable guidance on how to suspend correctly.

As far as all the extra status and all that, I think that's just an implementation detail inside use for how they track the status of the promise. If in the future if switch to use, we can replace throw value; with use(value), and then it will handle the promise side.

Our implementation can be less complex because the observable will update to the value after the promise resolves, which triggers useSelector to run again, which then returns the value instead of throwing the Promise. React's use needs to follow the promise because they're not already observing the promise like we are.

Also I believe the case for throwing errors is that Suspense should still render the fallback if the promise rejects. That's what use does under the hood.

So I think all we need for Suspense support is the above change to useSelector and a tweak to our observable.use() function to pass the options through.

[texastoland]

I don't think you directly adressed any of my replies. I'm happy to explain a few misunderstandings on Discord or something. Anyway best of luck it's a nice library!

[jmeistrich]

Sorry for the confusion, I thought I was addressing your replies 😂. I should probably make a habit of quoting replies more often.

IIUC you want to refactor promise handling out of createObservable/set into useSelector right? But that wouldn't be nice DX for non-Suspense anymore 🙀 See examples further down.

I was trying to clarify my proposal, that it wouldn't change any core behavior but just be a change in useSelector.

As far as all the extra status and all that, I think that's just an implementation detail inside use for how they track the status of the promise. If in the future if switch to use, we can replace throw value; with use(value), and then it will handle the promise side.

And it seemed like you were trying to give React promises with all the status they set up in use and I was trying to explain why I don't think that's necessary.

I'd be happy to chat more here or on our Slack if you'd like to join it. I definitely didn't mean to be dismissive. I would very much like to know what you think about the proposed change to useSelector - is it missing something or do you think that would work well?

[chakrihacker]

Nice to see suspense support for legend state, I like what @jmeistrich suggested which is adding to options

[jmeistrich]

I released the updated useSelector hook and use({ suspend: true }) in version 1.8.0. Please let me know how that works for you 😀