Skip to content

Commit

Permalink
Delete flushSuspenseFallbacksInTests
Browse files Browse the repository at this point in the history 
This was meant to be a temporary hack to unblock the `act` work, but it
quickly spread throughout our tests.

What it's meant to do is force fallbacks to flush inside `act` even in
Concurrent Mode. It does this by wrapping the `setTimeout` call in a
check to see if it's in an `act` context. If so, it skips the delay and
immediately commits the fallback.

Really this is only meant for our internal React tests that need to
incrementally render. Nobody outside our team (and Relay) needs to do
that, yet. Even if/when we do support that, it may or may not be with
the same `flushAndYield` pattern we use internally.

However, even for our internal purposes, the behavior isn't right
because a really common reason we flush work incrementally is to make
assertions on the "suspended" state, before the fallback has committed.
There's no way to do that from inside `act` with the behavior of this
flag, because it causes the fallback to immediately commit. This has led
us to *not* use `act` in a lot of our tests, or to write code that
doesn't match what would actually happen in a real environment.

What we really want is for the fallbacks to be flushed at the *end` of
the `act` scope. Not within it.

This only affects the noop and test renderer versions of `act`, which
are implemented inside the reconciler. Whereas `ReactTestUtils.act` is
implemented in "userspace" for backwards compatibility. This is fine
because we didn't have any DOM Suspense tests that relied on this flag;
they all use test renderer or noop.

In the future, we'll probably want to move always use the reconciler
implementation of `act`. It will not affect the prod bundle, because we
currently only plan to support `act` in dev. Though we still haven't
completely figured that out. However, regardless of whether we support a
production `act` for users, we'll still need to write internal React
tests in production mode. For that use case, we'll likely add our own
internal version of `act` that assumes a mock Scheduler and might rely
on hacks that don't 100% align up with the public one.
  • Loading branch information
@acdlite
acdlite committed Apr 14, 2020
1 parent c2c2cf5 commit c87f73e
Show file tree
Hide file tree
Showing 15 changed files with 334 additions and 294 deletions.
4 changes: 0 additions & 4 deletions packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils');

let React;
let ReactFeatureFlags;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;
Expand All @@ -39,9 +38,6 @@ function initModules() {
// Reset warning cache.
jest.resetModuleRegistry();

ReactFeatureFlags = require('shared/ReactFeatureFlags');

ReactFeatureFlags.flushSuspenseFallbacksInTests = false;
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
Expand Down
88 changes: 58 additions & 30 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {
enableProfilerCommitHooks,
enableSchedulerTracing,
warnAboutUnmockedScheduler,
flushSuspenseFallbacksInTests,
disableSchedulerTimeoutBasedOnReactExpirationTime,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
Expand Down Expand Up @@ -742,11 +741,7 @@ function finishConcurrentRender(
if (
hasNotProcessedNewUpdates &&
// do not delay if we're inside an act() scope
!(
__DEV__ &&
flushSuspenseFallbacksInTests &&
IsThisRendererActing.current
)
!shouldForceFlushFallbacksInDEV()
) {
// If we have not processed any new updates during this pass, then
// this is either a retry of an existing fallback state or a
Expand Down Expand Up @@ -805,11 +800,7 @@ function finishConcurrentRender(

if (
// do not delay if we're inside an act() scope
!(
__DEV__ &&
flushSuspenseFallbacksInTests &&
IsThisRendererActing.current
)
!shouldForceFlushFallbacksInDEV()
) {
// We're suspended in a state that should be avoided. We'll try to
// avoid committing it for as long as the timeouts let us.
Expand Down Expand Up @@ -881,11 +872,7 @@ function finishConcurrentRender(
// The work completed. Ready to commit.
if (
// do not delay if we're inside an act() scope
!(
__DEV__ &&
flushSuspenseFallbacksInTests &&
IsThisRendererActing.current
) &&
!shouldForceFlushFallbacksInDEV() &&
workInProgressRootLatestProcessedEventTime !== Sync &&
workInProgressRootCanSuspendUsingConfig !== null
) {
Expand Down Expand Up @@ -3206,24 +3193,62 @@ function finishPendingInteractions(root, committedExpirationTime) {
// TODO: This is mostly a copy-paste from the legacy `act`, which does not have
// access to the same internals that we do here. Some trade offs in the
// implementation no longer make sense.
const isSchedulerMocked =
typeof Scheduler.unstable_flushAllWithoutAsserting === 'function';
const flushSchedulerWork =
Scheduler.unstable_flushAllWithoutAsserting ||
function() {
let didFlushWork = false;
while (flushPassiveEffects()) {
didFlushWork = true;
}

return didFlushWork;
};
let isFlushingAct = false;
let isInsideThisAct = false;

// TODO: Yes, this is confusing. See above comment. We'll refactor it.
function shouldForceFlushFallbacksInDEV() {
if (!__DEV__) {
// Never force flush in production. This function should get stripped out.
return false;
}
// `IsThisRendererActing.current` is used by ReactTestUtils version of `act`.
if (IsThisRendererActing.current) {
// `isInsideAct` is only used by the reconciler implementation of `act`.
// We don't want to flush suspense fallbacks until the end.
return !isInsideThisAct;
}
// Flush callbacks at the end.
return isFlushingAct;
}

const flushMockScheduler = Scheduler.unstable_flushAllWithoutAsserting;
const isSchedulerMocked = typeof flushMockScheduler === 'function';

// Returns whether additional work was scheduled. Caller should keep flushing
// until there's no work left.
function flushActWork(): boolean {
if (flushMockScheduler !== undefined) {
const prevIsFlushing = isFlushingAct;
isFlushingAct = true;
try {
return flushMockScheduler();
} finally {
isFlushingAct = prevIsFlushing;
}
} else {
// No mock scheduler available. However, the only type of pending work is
// passive effects, which we control. So we can flush that.
const prevIsFlushing = isFlushingAct;
isFlushingAct = true;
try {
let didFlushWork = false;
while (flushPassiveEffects()) {
didFlushWork = true;
}
return didFlushWork;
} finally {
isFlushingAct = prevIsFlushing;
}
}
}

function flushWorkAndMicroTasks(onDone: (err: ?Error) => void) {
try {
flushSchedulerWork();
flushActWork();
enqueueTask(() => {
if (flushSchedulerWork()) {
if (flushActWork()) {
flushWorkAndMicroTasks(onDone);
} else {
onDone();
Expand Down Expand Up @@ -3256,13 +3281,16 @@ export function act(callback: () => Thenable<mixed>): Thenable<void> {

const previousIsSomeRendererActing = IsSomeRendererActing.current;
const previousIsThisRendererActing = IsThisRendererActing.current;
const previousIsInsideThisAct = isInsideThisAct;
IsSomeRendererActing.current = true;
IsThisRendererActing.current = true;
isInsideThisAct = true;

function onDone() {
actingUpdatesScopeDepth--;
IsSomeRendererActing.current = previousIsSomeRendererActing;
IsThisRendererActing.current = previousIsThisRendererActing;
isInsideThisAct = previousIsInsideThisAct;
if (__DEV__) {
if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
// if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
Expand Down Expand Up @@ -3362,7 +3390,7 @@ export function act(callback: () => Thenable<mixed>): Thenable<void> {
) {
// we're about to exit the act() scope,
// now's the time to flush effects
flushSchedulerWork();
flushActWork();
}
onDone();
} catch (err) {
Expand Down
88 changes: 58 additions & 30 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {
enableProfilerCommitHooks,
enableSchedulerTracing,
warnAboutUnmockedScheduler,
flushSuspenseFallbacksInTests,
disableSchedulerTimeoutBasedOnReactExpirationTime,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
Expand Down Expand Up @@ -735,11 +734,7 @@ function finishConcurrentRender(
if (
hasNotProcessedNewUpdates &&
// do not delay if we're inside an act() scope
!(
__DEV__ &&
flushSuspenseFallbacksInTests &&
IsThisRendererActing.current
)
!shouldForceFlushFallbacksInDEV()
) {
// If we have not processed any new updates during this pass, then
// this is either a retry of an existing fallback state or a
Expand Down Expand Up @@ -798,11 +793,7 @@ function finishConcurrentRender(

if (
// do not delay if we're inside an act() scope
!(
__DEV__ &&
flushSuspenseFallbacksInTests &&
IsThisRendererActing.current
)
!shouldForceFlushFallbacksInDEV()
) {
// We're suspended in a state that should be avoided. We'll try to
// avoid committing it for as long as the timeouts let us.
Expand Down Expand Up @@ -889,11 +880,7 @@ function finishConcurrentRender(
// The work completed. Ready to commit.
if (
// do not delay if we're inside an act() scope
!(
__DEV__ &&
flushSuspenseFallbacksInTests &&
IsThisRendererActing.current
) &&
!shouldForceFlushFallbacksInDEV() &&
workInProgressRootLatestProcessedExpirationTime !== Sync &&
workInProgressRootCanSuspendUsingConfig !== null
) {
Expand Down Expand Up @@ -3228,24 +3215,62 @@ function finishPendingInteractions(root, committedExpirationTime) {
// TODO: This is mostly a copy-paste from the legacy `act`, which does not have
// access to the same internals that we do here. Some trade offs in the
// implementation no longer make sense.
const isSchedulerMocked =
typeof Scheduler.unstable_flushAllWithoutAsserting === 'function';
const flushSchedulerWork =
Scheduler.unstable_flushAllWithoutAsserting ||
function() {
let didFlushWork = false;
while (flushPassiveEffects()) {
didFlushWork = true;
}

return didFlushWork;
};
let isFlushingAct = false;
let isInsideThisAct = false;

// TODO: Yes, this is confusing. See above comment. We'll refactor it.
function shouldForceFlushFallbacksInDEV() {
if (!__DEV__) {
// Never force flush in production. This function should get stripped out.
return false;
}
// `IsThisRendererActing.current` is used by ReactTestUtils version of `act`.
if (IsThisRendererActing.current) {
// `isInsideAct` is only used by the reconciler implementation of `act`.
// We don't want to flush suspense fallbacks until the end.
return !isInsideThisAct;
}
// Flush callbacks at the end.
return isFlushingAct;
}

const flushMockScheduler = Scheduler.unstable_flushAllWithoutAsserting;
const isSchedulerMocked = typeof flushMockScheduler === 'function';

// Returns whether additional work was scheduled. Caller should keep flushing
// until there's no work left.
function flushActWork(): boolean {
if (flushMockScheduler !== undefined) {
const prevIsFlushing = isFlushingAct;
isFlushingAct = true;
try {
return flushMockScheduler();
} finally {
isFlushingAct = prevIsFlushing;
}
} else {
// No mock scheduler available. However, the only type of pending work is
// passive effects, which we control. So we can flush that.
const prevIsFlushing = isFlushingAct;
isFlushingAct = true;
try {
let didFlushWork = false;
while (flushPassiveEffects()) {
didFlushWork = true;
}
return didFlushWork;
} finally {
isFlushingAct = prevIsFlushing;
}
}
}

function flushWorkAndMicroTasks(onDone: (err: ?Error) => void) {
try {
flushSchedulerWork();
flushActWork();
enqueueTask(() => {
if (flushSchedulerWork()) {
if (flushActWork()) {
flushWorkAndMicroTasks(onDone);
} else {
onDone();
Expand Down Expand Up @@ -3278,13 +3303,16 @@ export function act(callback: () => Thenable<mixed>): Thenable<void> {

const previousIsSomeRendererActing = IsSomeRendererActing.current;
const previousIsThisRendererActing = IsThisRendererActing.current;
const previousIsInsideThisAct = isInsideThisAct;
IsSomeRendererActing.current = true;
IsThisRendererActing.current = true;
isInsideThisAct = true;

function onDone() {
actingUpdatesScopeDepth--;
IsSomeRendererActing.current = previousIsSomeRendererActing;
IsThisRendererActing.current = previousIsThisRendererActing;
isInsideThisAct = previousIsInsideThisAct;
if (__DEV__) {
if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
// if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
Expand Down Expand Up @@ -3384,7 +3412,7 @@ export function act(callback: () => Thenable<mixed>): Thenable<void> {
) {
// we're about to exit the act() scope,
// now's the time to flush effects
flushSchedulerWork();
flushActWork();
}
onDone();
} catch (err) {
Expand Down
Loading

0 comments on commit c87f73e

Please sign in to comment.