Skip to content
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

Merged
merged 1 commit into from Nov 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
107 changes: 53 additions & 54 deletions packages/react-reconciler/src/ReactFiberHooks.js
Expand Up @@ -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';
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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],
);
Expand All @@ -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(
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

priorityLevel < UserBlockingPriority ? UserBlockingPriority : priorityLevel,
() => {
setPending(true);
},
);
runWithPriority(
priorityLevel > NormalPriority ? NormalPriority : priorityLevel,
Copy link
Collaborator

Choose a reason for hiding this comment

The 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>(
Expand Down
@@ -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', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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');
});
});