Skip to content

Commit

Permalink
Delete flushSuspenseFallbacksInTests flag (#18596)
Browse files Browse the repository at this point in the history 
* Move renderer `act` to work loop

* Delete `flushSuspenseFallbacksInTests`

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 f3f3d77 commit b928fc0
Show file tree
Hide file tree
Showing 18 changed files with 721 additions and 676 deletions.
31 changes: 20 additions & 11 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 Expand Up @@ -1281,13 +1277,26 @@ describe('ReactDOMServerHooks', () => {

// State update should trigger the ID to update, which changes the props
// of ChildWithID. This should cause ChildWithID to hydrate before Children
expect(Scheduler).toFlushAndYieldThrough([
'Child with ID',
'Child with ID',
'Child with ID',
'Child One',
'Child Two',
]);

expect(Scheduler).toFlushAndYieldThrough(
__DEV__
? [
'Child with ID',
// Fallbacks are immdiately committed in TestUtils version
// of act
// 'Child with ID',
// 'Child with ID',
'Child One',
'Child Two',
]
: [
'Child with ID',
'Child with ID',
'Child with ID',
'Child One',
'Child Two',
],
);

expect(child1Ref.current).toBe(null);
expect(childWithIDRef.current).toEqual(
Expand Down
189 changes: 3 additions & 186 deletions packages/react-reconciler/src/ReactFiberReconciler.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import type {
} from './ReactFiberHostConfig';
import type {RendererInspectionConfig} from './ReactFiberHostConfig';
import {FundamentalComponent} from './ReactWorkTags';
import type {ReactNodeList, Thenable} from 'shared/ReactTypes';
import type {ReactNodeList} from 'shared/ReactTypes';
import type {ExpirationTime} from './ReactFiberExpirationTime.new';
import type {SuspenseState} from './ReactFiberSuspenseComponent.new';

Expand Down Expand Up @@ -64,6 +64,7 @@ import {
warnIfNotScopedWithMatchingAct,
warnIfUnmockedScheduler,
IsThisRendererActing,
act,
} from './ReactFiberWorkLoop.new';
import {createUpdate, enqueueUpdate} from './ReactUpdateQueue.new';
import {getStackByFiberInDevAndProd} from './ReactFiberComponentStack';
Expand All @@ -85,11 +86,6 @@ import {
findHostInstancesForRefresh,
} from './ReactFiberHotReloading.new';

// used by isTestEnvironment builds
import enqueueTask from 'shared/enqueueTask';
import * as Scheduler from 'scheduler';
// end isTestEnvironment imports

export {createPortal} from './ReactPortal';

type OpaqueRoot = FiberRoot;
Expand Down Expand Up @@ -308,6 +304,7 @@ export {
flushSync,
flushPassiveEffects,
IsThisRendererActing,
act,
};

export function getPublicRootInstance(
Expand Down Expand Up @@ -547,183 +544,3 @@ export function injectIntoDevTools(devToolsConfig: DevToolsConfig): boolean {
getCurrentFiber: __DEV__ ? getCurrentFiberForDevTools : null,
});
}

const {IsSomeRendererActing} = ReactSharedInternals;
const isSchedulerMocked =
typeof Scheduler.unstable_flushAllWithoutAsserting === 'function';
const flushWork =
Scheduler.unstable_flushAllWithoutAsserting ||
function() {
let didFlushWork = false;
while (flushPassiveEffects()) {
didFlushWork = true;
}

return didFlushWork;
};

function flushWorkAndMicroTasks(onDone: (err: ?Error) => void) {
try {
flushWork();
enqueueTask(() => {
if (flushWork()) {
flushWorkAndMicroTasks(onDone);
} else {
onDone();
}
});
} catch (err) {
onDone(err);
}
}

// we track the 'depth' of the act() calls with this counter,
// so we can tell if any async act() calls try to run in parallel.

let actingUpdatesScopeDepth = 0;
let didWarnAboutUsingActInProd = false;

// eslint-disable-next-line no-inner-declarations
export function act(callback: () => Thenable<mixed>): Thenable<void> {
if (!__DEV__) {
if (didWarnAboutUsingActInProd === false) {
didWarnAboutUsingActInProd = true;
// eslint-disable-next-line react-internal/no-production-logging
console.error(
'act(...) is not supported in production builds of React, and might not behave as expected.',
);
}
}

const previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
actingUpdatesScopeDepth++;

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

function onDone() {
actingUpdatesScopeDepth--;
IsSomeRendererActing.current = previousIsSomeRendererActing;
IsThisRendererActing.current = previousIsThisRendererActing;
if (__DEV__) {
if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
// if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
console.error(
'You seem to have overlapping act() calls, this is not supported. ' +
'Be sure to await previous act() calls before making a new one. ',
);
}
}
}

let result;
try {
result = batchedUpdates(callback);
} catch (error) {
// on sync errors, we still want to 'cleanup' and decrement actingUpdatesScopeDepth
onDone();
throw error;
}

if (
result !== null &&
typeof result === 'object' &&
typeof result.then === 'function'
) {
// setup a boolean that gets set to true only
// once this act() call is await-ed
let called = false;
if (__DEV__) {
if (typeof Promise !== 'undefined') {
//eslint-disable-next-line no-undef
Promise.resolve()
.then(() => {})
.then(() => {
if (called === false) {
console.error(
'You called act(async () => ...) without await. ' +
'This could lead to unexpected testing behaviour, interleaving multiple act ' +
'calls and mixing their scopes. You should - await act(async () => ...);',
);
}
});
}
}

// in the async case, the returned thenable runs the callback, flushes
// effects and microtasks in a loop until flushPassiveEffects() === false,
// and cleans up
return {
then(resolve, reject) {
called = true;
result.then(
() => {
if (
actingUpdatesScopeDepth > 1 ||
(isSchedulerMocked === true &&
previousIsSomeRendererActing === true)
) {
onDone();
resolve();
return;
}
// we're about to exit the act() scope,
// now's the time to flush tasks/effects
flushWorkAndMicroTasks((err: ?Error) => {
onDone();
if (err) {
reject(err);
} else {
resolve();
}
});
},
err => {
onDone();
reject(err);
},
);
},
};
} else {
if (__DEV__) {
if (result !== undefined) {
console.error(
'The callback passed to act(...) function ' +
'must return undefined, or a Promise. You returned %s',
result,
);
}
}

// flush effects until none remain, and cleanup
try {
if (
actingUpdatesScopeDepth === 1 &&
(isSchedulerMocked === false || previousIsSomeRendererActing === false)
) {
// we're about to exit the act() scope,
// now's the time to flush effects
flushWork();
}
onDone();
} catch (err) {
onDone();
throw err;
}

// in the sync case, the returned thenable only warns *if* await-ed
return {
then(resolve) {
if (__DEV__) {
console.error(
'Do not await the result of calling act(...) with sync logic, it is not a Promise.',
);
}
resolve();
},
};
}
}
Loading

0 comments on commit b928fc0

Please sign in to comment.