Capture suspense boundaries with undefined fallbacks #21702

rickhanlonii · 2021-06-18T17:20:32Z

Overview

This PR updates the behavior of the Suspense component so that it when the fallback is missing we still capture the boundary and renders null for the fallback.

Consider this code:

<Suspense fallback="outer"}>
  <Suspense>
    <AsyncText text="A"/>
  </Suspense>
</Suspense>,

Previously, since the inner Suspense boundary did not provide a fallback, we silently skipped it and kept going to the "outer" boundary. This is hard to debug and can be confusing.

Instead, we're going to capture the inner boundary and render an empty placeholder. This should make it more obvious to the user that they forgot to provide the fallback, and allow supporting use cases where that's what you intend to do.

rickhanlonii · 2021-06-18T17:21:25Z

packages/react-dom/src/server/ReactPartialRenderer.js

-              }
-              this.stack.push(frame);
-              return '';
-            }


I could not find a test that exercises this block, not could I figure out how to create one.

I'll make one. I had that on my TODO to test this parity.

rickhanlonii · 2021-06-18T17:22:12Z

packages/react-reconciler/src/ReactFiberBeginWork.new.js

@@ -1910,22 +1906,18 @@ function updateSuspenseComponent(current, workInProgress, renderLanes) {
  if (current === null) {
    // Initial mount
    // If we're currently hydrating, try to hydrate this boundary.
-    // But only if this has a fallback.


This change is reflected in the ReactDOMServerPartialHydration-test.internal.js test.

rickhanlonii · 2021-06-18T17:22:42Z

packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js

@@ -391,44 +391,10 @@ describe('ReactSuspense', () => {
    expect(root).toMatchRenderedOutput('Hi');
  });

-  it('only captures if `fallback` is defined', () => {


Moved to ReactSuspenseFallback-test.

sizebot · 2021-06-18T17:25:34Z

Comparing: 43f4cc1...614f1c6

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	127.34 kB	127.22 kB	=	40.82 kB	40.78 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	130.16 kB	130.03 kB	=	41.73 kB	41.73 kB
facebook-www/ReactDOM-prod.classic.js	=	405.75 kB	405.48 kB	=	75.02 kB	74.99 kB
facebook-www/ReactDOM-prod.modern.js	=	394.18 kB	393.91 kB	=	73.24 kB	73.20 kB
facebook-www/ReactDOMForked-prod.classic.js	=	405.75 kB	405.48 kB	=	75.03 kB	74.99 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
facebook-www/ReactDOMServer-dev.classic.js	=	153.44 kB	152.83 kB	=	39.00 kB	38.91 kB
facebook-www/ReactDOMServer-prod.classic.js	=	48.21 kB	47.87 kB	=	11.24 kB	11.20 kB

Generated by 🚫 dangerJS against 614f1c6

packages/react-reconciler/src/__tests__/ReactSuspenseFallback-test.js

acdlite · 2021-06-18T18:13:32Z

packages/react-reconciler/src/__tests__/ReactSuspenseFallback-test.js

+    React = require('react');
+    ReactTestRenderer = require('react-test-renderer');
+    Scheduler = require('scheduler');
+    ReactCache = require('react-cache');


Can you use the helpers in ReactSuspenseWithNoopRenderer-test or ReactCache-test instead? Those are the closest to our preferred testing idioms. We're going to remove the react-cache package.

Ideally just put them into those existing files. It tends to get annoying to update the helpers in different files that use the same patterns. For example we'll need to do a bunch of refactoring to change the avoidThisFallback API that needs to basically test all the same things again.

I put them in separate files because @acdlite mentioned that we should break out more tests into separate files for speed and organizational reasons.

In that case, maybe the helpers should be broken out into a shared module too.

Organizationally I find that what organization we thought would make sense in terms of grouping features doesn't hold up for the next change.

The defacto pattern that has evolved that is pretty convenient is that files are grouped by the kind of test mechanism. So you can find a test with similar mechanism that you need and then copy it. It doesn't help speed so the way we solved that for the ServerIntegration tests is that we broke them out into a shared helper and then pulled that in everywhere.

It would also be nice if we didn't mix test mechanisms in the same file.

My main reason for suggesting more focused module was because some of them are getting really slow.

I've been resistant to the idea of having test helper modules because I don't want it to become a dumping ground for a bunch of idiosyncratic abstractions. I like that if there's a bad abstraction, it's confined to a single module.

Really it's because I dislike when a test breaks and I have to open multiple files to figure out what the hell is going on, which is the case for some of our older server rendering tests

The scheduler.yield things are a helper that is an example of doing it right so it can be done right.

I'm not a fan of the server rendering strategy per se but at least it's the same one every time. The ones that get really frustrating to upgrade from an older pattern are the ones using similar but slightly different patterns.

Who am I and what have I done to the good olde wet Seb?

Ultimately this is kind of a flaw of jest. We reset modules between every run anyway. There's no reason we couldn't run every individual test case in a separate process. That would also let us use proper ES modules since it would by default reset modules.

The ones that get really frustrating to upgrade from an older pattern are the ones using similar but slightly different patterns.

I agree with that but I suppose I don't mind because in aggregate we spend more time debugging tests than we do making API changes.

In this particular case, though, I think the "async text" pattern has solidified enough that I'd be comfortable moving it to its own helper, a la yieldValue.

bvaughn · 2021-07-13T00:43:32Z

Hey @rickhanlonii @acdlite Did you mean to merge this into master? I assume you meant main

Capture suspense boundaries with undefined fallbacks (facebook#21702)

zpao · 2021-07-13T06:11:31Z

He fixed it with #21854. I am curious how master got re-involved here. The target ref should have been updated to main when the rename happened…

rickhanlonii · 2021-07-24T17:06:28Z

@zpao there may have been a failure when doing the rename. I can't tell now because we deleted the branch, but you can see how in this old screenshot of react-native after the rename that there was a "rename failed" next to master:

bvaughn · 2021-07-26T13:39:57Z

Yeah, we later noticed that a lot of PRs failed to get renamed. I think the underlying cause has since been fixed (by Jon?)

rickhanlonii requested review from sebmarkbage and acdlite June 18, 2021 17:20

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 18, 2021

rickhanlonii changed the title ~~Capture suspense boundaries with undefined/null fallbacks~~ Capture suspense boundaries with undefined fallbacks Jun 18, 2021

rickhanlonii commented Jun 18, 2021

View reviewed changes

acdlite reviewed Jun 18, 2021

View reviewed changes

packages/react-reconciler/src/__tests__/ReactSuspenseFallback-test.js Outdated Show resolved Hide resolved

acdlite reviewed Jun 18, 2021

View reviewed changes

rickhanlonii force-pushed the rh-undefined-fallback branch 4 times, most recently from 37e6874 to b0c7eb3 Compare June 18, 2021 20:43

Capture suspense boundaries with undefined fallbacks

614f1c6

rickhanlonii force-pushed the rh-undefined-fallback branch from b0c7eb3 to 614f1c6 Compare June 18, 2021 21:00

rickhanlonii requested a review from acdlite June 18, 2021 21:33

acdlite approved these changes Jun 21, 2021

View reviewed changes

rickhanlonii merged commit dc4a9b5 into facebook:master Jul 12, 2021

rickhanlonii deleted the rh-undefined-fallback branch July 12, 2021 18:26

rickhanlonii restored the rh-undefined-fallback branch July 12, 2021 18:27

sthagen added a commit to sthagen/facebook-react that referenced this pull request Jul 13, 2021

Merge pull request #53 from facebook/master

9aa6b05

Capture suspense boundaries with undefined fallbacks (facebook#21702)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capture suspense boundaries with undefined fallbacks #21702

Capture suspense boundaries with undefined fallbacks #21702

rickhanlonii commented Jun 18, 2021

rickhanlonii Jun 18, 2021

sebmarkbage Jun 18, 2021

rickhanlonii Jun 18, 2021

rickhanlonii Jun 18, 2021

sizebot commented Jun 18, 2021 •

edited

Loading

acdlite Jun 18, 2021

sebmarkbage Jun 18, 2021

rickhanlonii Jun 18, 2021

sebmarkbage Jun 18, 2021

acdlite Jun 18, 2021

acdlite Jun 18, 2021

sebmarkbage Jun 18, 2021

sebmarkbage Jun 18, 2021

acdlite Jun 21, 2021

bvaughn commented Jul 13, 2021

zpao commented Jul 13, 2021

rickhanlonii commented Jul 24, 2021

bvaughn commented Jul 26, 2021

Capture suspense boundaries with undefined fallbacks #21702

Capture suspense boundaries with undefined fallbacks #21702

Conversation

rickhanlonii commented Jun 18, 2021

Overview

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sizebot commented Jun 18, 2021 • edited Loading

Critical size changes

Significant size changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Jul 13, 2021

zpao commented Jul 13, 2021

rickhanlonii commented Jul 24, 2021

bvaughn commented Jul 26, 2021

sizebot commented Jun 18, 2021 •

edited

Loading