-
Notifications
You must be signed in to change notification settings - Fork 47k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bugfix] Pending state is always user-blocking #17382
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<T>( | |
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<T>( | |
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly here, it's not obvious to me that calling startTransition should change the running "current" queue of the scheduler. It should only change the priority inside React. They're not necessarily 1:1. |
||
() => { | ||
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<S, A>( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copy paste name |
||
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 <Text text={text} />; | ||
}; | ||
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 ( | ||
<Suspense fallback={<Text text="Loading..." />}> | ||
{isPending ? <Text text="Pending..." /> : null} | ||
{show ? <Async /> : <Text text="(empty)" />} | ||
</Suspense> | ||
); | ||
} | ||
|
||
const root = ReactNoop.createRoot(); | ||
|
||
await act(async () => { | ||
root.render(<App />); | ||
}); | ||
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'); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we do this in a couple of internal places which seems like a very heavy weight way to do this. Going through the scheduler to do this. This adds to my suspicion that reading priorities from scheduler is the wrong model. It's not that scheduler dictates the priorities but it sometimes can be used as a default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it's on my list of things to consider during expiration times refactor. Didn't want to couple it to this bugfix.