Triggering suspense with rejected promise causes re-render instead of error boundary

[joeldenning]

Do you want to request a feature or report a bug?

This might be a bug. @gaearon and @sebmarkbage shared differing opinions on it in this twitter thread

What is the current behavior?

If you throw a promise that rejects from a react component's render function, that rejection will be completely ignored. It will not show up in browser console, nor will it trigger a React error boundary. Instead, it will trigger a re-render (the same as if the promise had resolved).

Codepen example

What is the expected behavior?

My expectation was that the error boundary would be hit and the component would not re-render. Sebastian's tweet indicates that that is not the desired behavior, though.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

This impacts react@experimental, and also react@>=16.9.0

[vkurchatkin]

It seems that this is actually the intended behaviour. See example from the docs: https://codesandbox.io/s/adoring-goodall-8wbn7

Basically, if you want to handle data fetching error with an ErrorBoundary, you need to throw the error synchronously on subsequent rerender. This is what the example does:

function wrapPromise(promise) {
  let status = "pending";
  let result;
  let suspender = promise.then(
    r => {
      status = "success";
      result = r;
    },
    e => {
      status = "error";
      result = e;
    }
  );
  return {
    read() {
      if (status === "pending") {
        throw suspender; .// this is what happens on the first render
      } else if (status === "error") {
        throw result; // this is what happens when the promise rejects
      } else if (status === "success") {
        return result;
      }
    }
  };
}

Worth noting that the example explicitly says not to copy-paste this code

[sebmarkbage]

Note that I think it's quite likely that this won't be a public API at all. The public API will be the wrapper around this mechanism so the answer is that it doesn't matter and is an implementation detail of React.

[joeldenning]

What is the reasoning behind this implementation detail? Are there situations where it’s advantageous to the library author for a rejected promise to trigger a re-render instead of error boundary? To me, it seems like if a library author wants to trigger a re-render that they could just call .catch() and store the error without rejecting the promise. I’m not sure which functionality is possible due to this implementation detail that would become impossible without it.

[sebmarkbage]

The mechanism just needs a callback really. We kind of abused the Promise object for this purpose but it causes more confusion as a result. That’s why we’ll likely just switch to a different API for this.

The wrapper could definitely accept a promise and rejecting it would cause it to trigger an error boundary.

The point of the implementation details is that React may not store anything about where the promise was used. Including the error boundary. So depending on the details (such as if parent props have changed in the meantime) we may need to reexecute the renders to figure out which error boundary to trigger and what to render instead.

So semantically the real error comes from the function throwing an error after being reexecuted. This should be synchronous and not after one tick which is what would happen with a Promise.

[joeldenning]

👍 Thanks for the explanation, that makes sense. I am interested to see what the official public API becomes.

[Jack-Works]

I found this behavior is confusing when adopting Suspense mode. Here is my reproduction.

await import('https://cdn.jsdelivr.net/npm/[email protected]/umd/react.production.min.js')
await import('https://cdn.jsdelivr.net/npm/[email protected]/umd/react-dom.production.min.js')
const $ = React.createElement
function App() {
    const [input, setInput] = React.useState('')
    const [query, setQuery] = React.useState('')
    const translationResult = getTranslate(query)
    const [startTransition, isPending] = React.unstable_useTransition()
    return $('main', {},
        $('input', { onChange: e => { const val = e.currentTarget.value; setInput(val); startTransition(() => setQuery(val)) } }),
        $('br'),
        $('span', isPending ? { style: { color: 'gray' } } : {}, translationResult)
    )
}
const cache = new Map([['', '']])
function getTranslate(w) { if (cache.has(w)) return cache.get(w); else { throw translate(w) } }
async function translate(w) {if (w === 'error') throw new Error()
    await new Promise(r => setTimeout(r, 100))
    cache.set(w, [...w].reverse().join(''))
}
class ErrorBoundary extends React.Component {
  constructor(props) {
    super(props);
    this.state = { hasError: false };
  }
  static getDerivedStateFromError(error) {console.log(error)
    // Update state so the next render will show the fallback UI.
    return { hasError: true };
  }
  componentDidCatch(error, errorInfo) {}
  render() {
    if (this.state.hasError) {
      // You can render any custom fallback UI
      return $('h1', {}, 'Error!')
    }
    return this.props.children; 
  }
}
ReactDOM.unstable_createRoot(document.body.appendChild(document.createElement('main'))).render($(React.Suspense, { fallback: $('span', {}, 'loading') }, $(ErrorBoundary, {}, $(App))))

[Jack-Works]

Worth noting that the example explicitly says not to copy-paste this code

Why? That code seems useful

[gaearon]

Suspense has a very specific and strict contract. It's not currently documented, which is why we're asking not to build integrations with it except as an experiment. The examples above do not satisfy that contract.

Here's this contract in a nutshell. A Suspense-compatible cache should follow these rules:

• It returns the value (or throws an error) synchronously, if it’s fetched for the given key.
• It throws a Promise if a value is not fetched for the given key yet, and starts the fetch.
  • If data for the requested key is already being fetched, the same exact Promise must be thrown. (So the Promise itself is stored in the cache until it settles.)
  • When the Promise resolves, the cache is updated with either the result (so that it can be read successfully next time), or an error (so that it is rethrown next time).
    • Note that the Promise’s value is ignored by React. What matters is that we put the result into the cache so that React can read it during the retry.
  • The result is then cached. This cache is “append-only”: once we put the fetched data into the cache, we never mutate or remove it. (There is a separate mechanism for invalidation in React itself, which amounts to replacing the cache with an empty one — see Built-in Suspense Cache reactwg/react-18#25 for an early look.)

The bolded part explains that you're supposed to cache the errors, too. We'll document this contract when Suspense for data fetching is considered stable.

[gaearon]

(It's still possible we'll offer a higher-level API that enforces these constraints, but I wanted to document the current state.)

[Jack-Works]

thanks, @gaearon this contract explanation really helps me to solve the problem

[Jack-Works]

@gaearon hi can you check out this, is this a correct concurrent mode ready cache?
https://codesandbox.io/s/dreamy-elion-0kzkl

[Jack-Works]

1. All state should be stored in the useRef of useState hooks.

const someGlobalCache = new Map() // wrong
export function useMyHooks() {
    // do something with global cache
}

2. All states should be immutable (important!)

During the concurrent rendering, React will copy refs and states by reference (in my understanding). So any mutation will cause bugs in the concurrent mode.

function useX() {
    const ref = useRef({})
    useEffect(() => {
        ref.current.xyz = 1 // Wrong!
        ref.current = { ...ref.current, xyz: 1 } // Safe
    })
}

(Originally posted in react-hook-form/react-hook-form#2333 (comment))

Does my understanding right? @gaearon Thanks!

[gaearon]

All state should be stored in the useRef of useState hooks.

I don't know what you mean by this. If you mean the Cache, no, it should not be stored in either of these, because they get destroyed when Suspending before mount. We have some built-in helpers to hold the Cache coming later, but they're not a stable API yet. You can look at the Server Components demo to see how they can be wired up in experimental releases.

[gaearon]

Maybe this example can help:

https://codesandbox.io/s/wonderful-sinoussi-obo84?file=/src/App.js

Again, note these are experimental APIs and we don't recommend building anything on top of them at the moment.

[Jack-Works]

cool thanks, is this correct? https://codesandbox.io/s/dreamy-elion-0kzkl

[Jack-Works]

@gaearon https://60800555ef7c5d0007b443e1--adoring-bardeen-d823e6.netlify.app/ Hi can you check out this? Is this a correct implementation? Source code is here https://github.com/Jack-Works/suspense-demo/tree/main/src

Thanks!

[gaearon]

An idiomatic solution would use a Cache API — what’s the reason you’re going with a mutable source?

[gaearon]

Anyway you should probably file a new issue so we don’t spam this one.

[Jack-Works]

An idiomatic solution would use a Cache API — what’s the reason you’re going with a mutable source?

Oh because I don't learn that... I'll rewrite that with Cache API and open a new issue for it. Thanks