Fix for failed Suspense layout semantics

[bvaughn]

Resolves #21676. Fixes failing test added in #21677

When adding the new Suspense/Offscreen effects semantics, we intentionally left out the subtreeFlags checks since these values aren't 100% reliable and the check is just an optimization. (Refer to the // TODO (Offscreen) comments in the source code.)

It looks like #21386 (re)added one though, which was causing a failure case originally reported here:
reactwg/react-18#31

For now, this PR just removes that check and adds a follow up TODO comment. This is just a bandaid fix to allow us to resume rolling out the new Suspense layout semantics. A proper long-term fix would be to identify why the subtreeFlags are incorrect in this case.

[gaearon]

Beautiful. Thank you.

[sizebot]

Comparing: a0d2d1e...742d0b0

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.40 kB	127.34 kB	=	40.83 kB	40.82 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	130.21 kB	130.16 kB	=	41.73 kB	41.73 kB
facebook-www/ReactDOM-prod.classic.js	=	405.89 kB	405.75 kB	=	75.02 kB	75.02 kB
facebook-www/ReactDOM-prod.modern.js	=	394.32 kB	394.18 kB	=	73.24 kB	73.24 kB
facebook-www/ReactDOMForked-prod.classic.js	=	405.89 kB	405.75 kB	=	75.03 kB	75.03 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 742d0b0

[bvaughn]

That's interesting. We have a slight timing difference for when we destroy the layout effect in production tests too.

ReactTestRenderer.act(() => {
  _setShow(true);
});
expect(Scheduler).toHaveYielded([
  "Child 1",
  "Suspend! [Child 2]",
  "Loading...",
  // In development we destroy it here
  "destroy layout",
]);
jest.advanceTimersByTime(1000);
expect(Scheduler).toHaveYielded([
  // In production we destroy it here
  "destroy layout",
  "Promise resolved [Child 2]",
]);

I'm not sure why there's a difference in these two behaviors. I would not expect there to be. Looking at the stack, I see the following frames in DEV mode:

  at commitRoot (packages/react-reconciler/src/ReactFiberWorkLoop.new.js:1696:5)
  at finishConcurrentRender (packages/react-reconciler/src/ReactFiberWorkLoop.new.js:930:9)
  at performConcurrentWorkOnRoot (packages/react-reconciler/src/ReactFiberWorkLoop.new.js:820:5)
  at workLoop (packages/scheduler/src/forks/SchedulerMock.js:188:34)
  at flushWork (packages/scheduler/src/forks/SchedulerMock.js:141:16)
  at unstable_flushAllWithoutAsserting (packages/scheduler/src/forks/SchedulerMock.js:531:23)
  at flushActWork (packages/react-reconciler/src/ReactFiberWorkLoop.new.js:2948:14)

But in production mode I see:

  at commitRoot (packages/react-reconciler/src/ReactFiberWorkLoop.new.js:1696:5)
  at callback (node_modules/@jest/fake-timers/build/legacyFakeTimers.js:565:32)
  at FakeTimers._runTimerHandle (node_modules/@jest/fake-timers/build/legacyFakeTimers.js:601:9)
  at FakeTimers.advanceTimersByTime (node_modules/@jest/fake-timers/build/legacyFakeTimers.js:292:14)

So for some reason, it looks like commitRoot is being deferred behind a setTimeout in the prod tests.

In Dev, higher up in the call stack is finishConcurrentRender, which checks shouldForceFlushFallbacksInDEV(). So this is probably the difference. Kind of unfortunate that it manifests itself in an observably different way like this huh.