From c292c8370418877eb040a8cd210ceb26d52da47e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 15 Nov 2019 11:26:25 -0800 Subject: [PATCH] [Bugfix] Pending state is always user-blocking Fixes a bug where `isPending` is only set to `true` if `startTransition` is called from inside an input event. That's usually the case, but not always. Now it works regardless of where you call it. --- .../react-reconciler/src/ReactFiberHooks.js | 107 +++++++++--------- .../ReactTransition-test.internal.js | 102 +++++++++++++++++ 2 files changed, 155 insertions(+), 54 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/ReactTransition-test.internal.js diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 14d70b5c5c459..1e80abb8ef8ea 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -19,7 +19,6 @@ import type {HookEffectTag} from './ReactHookEffectTags'; import type {SuspenseConfig} from './ReactFiberSuspenseConfig'; import type {ReactPriorityLevel} from './SchedulerWithReactIntegration'; -import * as Scheduler from 'scheduler'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import {NoWork} from './ReactFiberExpirationTime'; @@ -53,7 +52,12 @@ import getComponentName from 'shared/getComponentName'; import is from 'shared/objectIs'; import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork'; import {requestCurrentSuspenseConfig} from './ReactFiberSuspenseConfig'; -import {getCurrentPriorityLevel} from './SchedulerWithReactIntegration'; +import { + UserBlockingPriority, + NormalPriority, + runWithPriority, + getCurrentPriorityLevel, +} from './SchedulerWithReactIntegration'; const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals; @@ -1135,15 +1139,13 @@ function mountDeferredValue( const [prevValue, setValue] = mountState(value); mountEffect( () => { - Scheduler.unstable_next(() => { - const previousConfig = ReactCurrentBatchConfig.suspense; - ReactCurrentBatchConfig.suspense = config === undefined ? null : config; - try { - setValue(value); - } finally { - ReactCurrentBatchConfig.suspense = previousConfig; - } - }); + const previousConfig = ReactCurrentBatchConfig.suspense; + ReactCurrentBatchConfig.suspense = config === undefined ? null : config; + try { + setValue(value); + } finally { + ReactCurrentBatchConfig.suspense = previousConfig; + } }, [value, config], ); @@ -1157,65 +1159,62 @@ function updateDeferredValue( const [prevValue, setValue] = updateState(value); updateEffect( () => { - Scheduler.unstable_next(() => { - const previousConfig = ReactCurrentBatchConfig.suspense; - ReactCurrentBatchConfig.suspense = config === undefined ? null : config; - try { - setValue(value); - } finally { - ReactCurrentBatchConfig.suspense = previousConfig; - } - }); + const previousConfig = ReactCurrentBatchConfig.suspense; + ReactCurrentBatchConfig.suspense = config === undefined ? null : config; + try { + setValue(value); + } finally { + ReactCurrentBatchConfig.suspense = previousConfig; + } }, [value, config], ); return prevValue; } +function startTransition(setPending, config, callback) { + const priorityLevel = getCurrentPriorityLevel(); + runWithPriority( + priorityLevel < UserBlockingPriority ? UserBlockingPriority : priorityLevel, + () => { + setPending(true); + }, + ); + runWithPriority( + priorityLevel > NormalPriority ? NormalPriority : priorityLevel, + () => { + const previousConfig = ReactCurrentBatchConfig.suspense; + ReactCurrentBatchConfig.suspense = config === undefined ? null : config; + try { + setPending(false); + callback(); + } finally { + ReactCurrentBatchConfig.suspense = previousConfig; + } + }, + ); +} + function mountTransition( config: SuspenseConfig | void | null, ): [(() => void) => void, boolean] { const [isPending, setPending] = mountState(false); - const startTransition = mountCallback( - callback => { - setPending(true); - Scheduler.unstable_next(() => { - const previousConfig = ReactCurrentBatchConfig.suspense; - ReactCurrentBatchConfig.suspense = config === undefined ? null : config; - try { - setPending(false); - callback(); - } finally { - ReactCurrentBatchConfig.suspense = previousConfig; - } - }); - }, - [config, isPending], - ); - return [startTransition, isPending]; + const start = mountCallback(startTransition.bind(null, setPending, config), [ + setPending, + config, + ]); + return [start, isPending]; } function updateTransition( config: SuspenseConfig | void | null, ): [(() => void) => void, boolean] { const [isPending, setPending] = updateState(false); - const startTransition = updateCallback( - callback => { - setPending(true); - Scheduler.unstable_next(() => { - const previousConfig = ReactCurrentBatchConfig.suspense; - ReactCurrentBatchConfig.suspense = config === undefined ? null : config; - try { - setPending(false); - callback(); - } finally { - ReactCurrentBatchConfig.suspense = previousConfig; - } - }); - }, - [config, isPending], - ); - return [startTransition, isPending]; + const start = updateCallback(startTransition.bind(null, setPending, config), [ + setPending, + config, + ]); + return [start, isPending]; } function dispatchAction( diff --git a/packages/react-reconciler/src/__tests__/ReactTransition-test.internal.js b/packages/react-reconciler/src/__tests__/ReactTransition-test.internal.js new file mode 100644 index 0000000000000..d6e26be4adc21 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactTransition-test.internal.js @@ -0,0 +1,102 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + * @jest-environment node + */ + +'use strict'; + +let ReactFeatureFlags; +let React; +let ReactNoop; +let Scheduler; +let Suspense; +let useState; +let useTransition; +let act; + +describe('ReactHooksWithNoopRenderer', () => { + beforeEach(() => { + jest.resetModules(); + + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.enableSchedulerTracing = true; + ReactFeatureFlags.flushSuspenseFallbacksInTests = false; + React = require('react'); + ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); + useState = React.useState; + useTransition = React.useTransition; + Suspense = React.Suspense; + act = ReactNoop.act; + }); + + function Text(props) { + Scheduler.unstable_yieldValue(props.text); + return props.text; + } + + function createAsyncText(text) { + let resolved = false; + let Component = function() { + if (!resolved) { + Scheduler.unstable_yieldValue('Suspend! [' + text + ']'); + throw promise; + } + return ; + }; + let promise = new Promise(resolve => { + Component.resolve = function() { + resolved = true; + return resolve(); + }; + }); + return Component; + } + + it('isPending works even if called from outside an input event', async () => { + const Async = createAsyncText('Async'); + let start; + function App() { + const [show, setShow] = useState(false); + const [startTransition, isPending] = useTransition(); + start = () => startTransition(() => setShow(true)); + return ( + }> + {isPending ? : null} + {show ? : } + + ); + } + + const root = ReactNoop.createRoot(); + + await act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['(empty)']); + expect(root).toMatchRenderedOutput('(empty)'); + + await act(async () => { + start(); + + expect(Scheduler).toFlushAndYield([ + 'Pending...', + '(empty)', + 'Suspend! [Async]', + 'Loading...', + ]); + + expect(root).toMatchRenderedOutput('Pending...(empty)'); + + await Async.resolve(); + }); + expect(Scheduler).toHaveYielded(['Async']); + expect(root).toMatchRenderedOutput('Async'); + }); +});