React: Suspense component should only capture if fallback prop is defined

[jaredpalmer]

[Edit by @acdlite: Decided in comments below that we will change the semantics so that a missing fallback prop means the exception should propagate to the next parent (like a rethrow). That way a Suspense component can specify other props like maxDuration without needing to provide a fallback, too.]

---

Do you want to request a feature or report a bug?
Bug
What is the current behavior?
<React.Suspense> does not warn you if you omit a fallback). While redundant for TS/Flow usage, I misspelled the fallback prop by accident in a playground and was tearing my hair out trying to figure out why things were not working as expected.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

Omit a fallback.

What is the expected behavior?
React should warn during development if fallback is undefined.

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

Master

[gaearon]

Send a PR?

[romainwn]

Hi, would like to give it a try, but quite new to the project itself (looking for a first PR on React)
So, my guess is that we should write a test, that uses toWarnDev (or toThrowError ?) in the ReactSuspense-test.internal.js or maybe in the ReactSuspenseWithNoopRenderer-test.internal.js file instead ?

[sebmarkbage]

<Suspense maxDuration={...}> has semantics even without a fallback. (We’re also going to add some kind of priority field.)

That means that we should at least check if either is defined.

However then if you have a maxDuration you could still miss it. The only thing that would catch it all is looping over the props. That’s what propTypes is for but we’ve kinda moved away from that by moving it out.

[acdlite]

@sebmarkbage The current semantic if fallback is not defined is to fallback to empty. Should we change the semantics so it only "captures" if a fallback prop is defined?

That’s what propTypes is for but we’ve kinda moved away from that by moving it out.

We still warn if you return undefined from a component, though. This feels like the same type of warning.

[sebmarkbage]

We still warn if you return undefined from a component, though. This feels like the same type of warning.

I'd agree if it was sufficient to check for the existence of a fallback property. However, in this case, that's a valid use case. So we can't.

We typically don't warn by looping over. That's like the difference between subtyping objects and exact objects.

[acdlite]

What’s this looping over thing? I don’t get it. Before calling reconcileChildren, check if the fallback children are undefined.

[sebmarkbage]

fallback should be optional.

<Suspense maxDuration={0}>
  <Suspense maxDuration={1000} fallback={<Spinner />}>
    <ThingThatSuspends />
  </Suspense>
</Suspense>

[sebmarkbage]

I guess you're saying that in this case the inner one will be the one that actually suspends and therefore only in that case will we try to render the content. So the warning can be delayed instead of eager.

[acdlite]

@sebmarkbage Yeah, either that or we change the semantics so that it only "captures" and shows the fallback if fallback is actually defined. If fallback is missing it's like a rethrow.

[sebmarkbage]

Yea, that's probably what we should do.

With the displayPriority field it will be important to be able to add priority hints to these things without a fallback.

[kopax]

Hi, I am having an issue:

  A React component suspended while rendering, but no fallback UI was specified.

import React, { Suspense, lazy } from 'react';
const Deferred = lazy(() => import('./index'));

export default (props) => (
  <Suspense>
    <Deferred {...props} />
  </Suspense>
);

We use this with react-router and we expect to keep our main page by not using the fallback props. Unfortunatly, the flow makes the first route unmounted.

I know this is very common scenario, does anybody have a recommendation to share for doing this?

Thanks in advance,

[gaearon]

we expect to keep our main page by not using the fallback props

What does this mean? If you want to "preserve" the current screen while transitioning, that will only be supported in Concurrent Mode (but you'd still need a fallback in that case).

See https://reactjs.org/blog/2018/11/27/react-16-roadmap.html for roadmap on availability of Concurrent Mode.