Skip to content

Commit b928fc0

Browse files
acdliteacdlite
authored
Delete flushSuspenseFallbacksInTests flag (facebook#18596)
* 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.
1 parent f3f3d77 commit b928fc0

18 files changed

+721
-676
lines changed

‎packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils');
1515

1616
let React;
17-
let ReactFeatureFlags;
1817
let ReactDOM;
1918
let ReactDOMServer;
2019
let ReactTestUtils;
@@ -39,9 +38,6 @@ function initModules() {
3938
// Reset warning cache.
4039
jest.resetModuleRegistry();
4140

42-
ReactFeatureFlags = require('shared/ReactFeatureFlags');
43-
44-
ReactFeatureFlags.flushSuspenseFallbacksInTests = false;
4541
React = require('react');
4642
ReactDOM = require('react-dom');
4743
ReactDOMServer = require('react-dom/server');
@@ -1281,13 +1277,26 @@ describe('ReactDOMServerHooks', () => {
12811277

12821278
// State update should trigger the ID to update, which changes the props
12831279
// of ChildWithID. This should cause ChildWithID to hydrate before Children
1284-
expect(Scheduler).toFlushAndYieldThrough([
1285-
'Child with ID',
1286-
'Child with ID',
1287-
'Child with ID',
1288-
'Child One',
1289-
'Child Two',
1290-
]);
1280+
1281+
expect(Scheduler).toFlushAndYieldThrough(
1282+
__DEV__
1283+
? [
1284+
'Child with ID',
1285+
// Fallbacks are immdiately committed in TestUtils version
1286+
// of act
1287+
// 'Child with ID',
1288+
// 'Child with ID',
1289+
'Child One',
1290+
'Child Two',
1291+
]
1292+
: [
1293+
'Child with ID',
1294+
'Child with ID',
1295+
'Child with ID',
1296+
'Child One',
1297+
'Child Two',
1298+
],
1299+
);
12911300

12921301
expect(child1Ref.current).toBe(null);
12931302
expect(childWithIDRef.current).toEqual(

‎packages/react-reconciler/src/ReactFiberReconciler.new.js

Lines changed: 3 additions & 186 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import type {
1818
} from './ReactFiberHostConfig';
1919
import type {RendererInspectionConfig} from './ReactFiberHostConfig';
2020
import {FundamentalComponent} from './ReactWorkTags';
21-
import type {ReactNodeList, Thenable} from 'shared/ReactTypes';
21+
import type {ReactNodeList} from 'shared/ReactTypes';
2222
import type {ExpirationTime} from './ReactFiberExpirationTime.new';
2323
import type {SuspenseState} from './ReactFiberSuspenseComponent.new';
2424

@@ -64,6 +64,7 @@ import {
6464
warnIfNotScopedWithMatchingAct,
6565
warnIfUnmockedScheduler,
6666
IsThisRendererActing,
67+
act,
6768
} from './ReactFiberWorkLoop.new';
6869
import {createUpdate, enqueueUpdate} from './ReactUpdateQueue.new';
6970
import {getStackByFiberInDevAndProd} from './ReactFiberComponentStack';
@@ -85,11 +86,6 @@ import {
8586
findHostInstancesForRefresh,
8687
} from './ReactFiberHotReloading.new';
8788

88-
// used by isTestEnvironment builds
89-
import enqueueTask from 'shared/enqueueTask';
90-
import * as Scheduler from 'scheduler';
91-
// end isTestEnvironment imports
92-
9389
export {createPortal} from './ReactPortal';
9490

9591
type OpaqueRoot = FiberRoot;
@@ -308,6 +304,7 @@ export {
308304
flushSync,
309305
flushPassiveEffects,
310306
IsThisRendererActing,
307+
act,
311308
};
312309

313310
export function getPublicRootInstance(
@@ -547,183 +544,3 @@ export function injectIntoDevTools(devToolsConfig: DevToolsConfig): boolean {
547544
getCurrentFiber: __DEV__ ? getCurrentFiberForDevTools : null,
548545
});
549546
}
550-
551-
const {IsSomeRendererActing} = ReactSharedInternals;
552-
const isSchedulerMocked =
553-
typeof Scheduler.unstable_flushAllWithoutAsserting === 'function';
554-
const flushWork =
555-
Scheduler.unstable_flushAllWithoutAsserting ||
556-
function() {
557-
let didFlushWork = false;
558-
while (flushPassiveEffects()) {
559-
didFlushWork = true;
560-
}
561-
562-
return didFlushWork;
563-
};
564-
565-
function flushWorkAndMicroTasks(onDone: (err: ?Error) => void) {
566-
try {
567-
flushWork();
568-
enqueueTask(() => {
569-
if (flushWork()) {
570-
flushWorkAndMicroTasks(onDone);
571-
} else {
572-
onDone();
573-
}
574-
});
575-
} catch (err) {
576-
onDone(err);
577-
}
578-
}
579-
580-
// we track the 'depth' of the act() calls with this counter,
581-
// so we can tell if any async act() calls try to run in parallel.
582-
583-
let actingUpdatesScopeDepth = 0;
584-
let didWarnAboutUsingActInProd = false;
585-
586-
// eslint-disable-next-line no-inner-declarations
587-
export function act(callback: () => Thenable<mixed>): Thenable<void> {
588-
if (!__DEV__) {
589-
if (didWarnAboutUsingActInProd === false) {
590-
didWarnAboutUsingActInProd = true;
591-
// eslint-disable-next-line react-internal/no-production-logging
592-
console.error(
593-
'act(...) is not supported in production builds of React, and might not behave as expected.',
594-
);
595-
}
596-
}
597-
598-
const previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
599-
actingUpdatesScopeDepth++;
600-
601-
const previousIsSomeRendererActing = IsSomeRendererActing.current;
602-
const previousIsThisRendererActing = IsThisRendererActing.current;
603-
IsSomeRendererActing.current = true;
604-
IsThisRendererActing.current = true;
605-
606-
function onDone() {
607-
actingUpdatesScopeDepth--;
608-
IsSomeRendererActing.current = previousIsSomeRendererActing;
609-
IsThisRendererActing.current = previousIsThisRendererActing;
610-
if (__DEV__) {
611-
if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
612-
// if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
613-
console.error(
614-
'You seem to have overlapping act() calls, this is not supported. ' +
615-
'Be sure to await previous act() calls before making a new one. ',
616-
);
617-
}
618-
}
619-
}
620-
621-
let result;
622-
try {
623-
result = batchedUpdates(callback);
624-
} catch (error) {
625-
// on sync errors, we still want to 'cleanup' and decrement actingUpdatesScopeDepth
626-
onDone();
627-
throw error;
628-
}
629-
630-
if (
631-
result !== null &&
632-
typeof result === 'object' &&
633-
typeof result.then === 'function'
634-
) {
635-
// setup a boolean that gets set to true only
636-
// once this act() call is await-ed
637-
let called = false;
638-
if (__DEV__) {
639-
if (typeof Promise !== 'undefined') {
640-
//eslint-disable-next-line no-undef
641-
Promise.resolve()
642-
.then(() => {})
643-
.then(() => {
644-
if (called === false) {
645-
console.error(
646-
'You called act(async () => ...) without await. ' +
647-
'This could lead to unexpected testing behaviour, interleaving multiple act ' +
648-
'calls and mixing their scopes. You should - await act(async () => ...);',
649-
);
650-
}
651-
});
652-
}
653-
}
654-
655-
// in the async case, the returned thenable runs the callback, flushes
656-
// effects and microtasks in a loop until flushPassiveEffects() === false,
657-
// and cleans up
658-
return {
659-
then(resolve, reject) {
660-
called = true;
661-
result.then(
662-
() => {
663-
if (
664-
actingUpdatesScopeDepth > 1 ||
665-
(isSchedulerMocked === true &&
666-
previousIsSomeRendererActing === true)
667-
) {
668-
onDone();
669-
resolve();
670-
return;
671-
}
672-
// we're about to exit the act() scope,
673-
// now's the time to flush tasks/effects
674-
flushWorkAndMicroTasks((err: ?Error) => {
675-
onDone();
676-
if (err) {
677-
reject(err);
678-
} else {
679-
resolve();
680-
}
681-
});
682-
},
683-
err => {
684-
onDone();
685-
reject(err);
686-
},
687-
);
688-
},
689-
};
690-
} else {
691-
if (__DEV__) {
692-
if (result !== undefined) {
693-
console.error(
694-
'The callback passed to act(...) function ' +
695-
'must return undefined, or a Promise. You returned %s',
696-
result,
697-
);
698-
}
699-
}
700-
701-
// flush effects until none remain, and cleanup
702-
try {
703-
if (
704-
actingUpdatesScopeDepth === 1 &&
705-
(isSchedulerMocked === false || previousIsSomeRendererActing === false)
706-
) {
707-
// we're about to exit the act() scope,
708-
// now's the time to flush effects
709-
flushWork();
710-
}
711-
onDone();
712-
} catch (err) {
713-
onDone();
714-
throw err;
715-
}
716-
717-
// in the sync case, the returned thenable only warns *if* await-ed
718-
return {
719-
then(resolve) {
720-
if (__DEV__) {
721-
console.error(
722-
'Do not await the result of calling act(...) with sync logic, it is not a Promise.',
723-
);
724-
}
725-
resolve();
726-
},
727-
};
728-
}
729-
}

0 commit comments

Comments
 (0)