Skip to content

Commit

Permalink
Unify InputDiscreteLane with SyncLane (facebook#20968)
Browse files Browse the repository at this point in the history
* Unify sync priority and input discrete

* Fix lint

* Use update lane instead

* Update sync lane labels
  • Loading branch information
rickhanlonii authored and koto committed Jun 15, 2021
1 parent 82337d5 commit 81880dc
Show file tree
Hide file tree
Showing 9 changed files with 31 additions and 85 deletions.
41 changes: 11 additions & 30 deletions packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -386,63 +386,44 @@ describe('ReactDOMFiberAsync', () => {
});

// @gate experimental
it('ignores discrete events on a pending removed element', () => {
it('ignores discrete events on a pending removed element', async () => {
const disableButtonRef = React.createRef();
const submitButtonRef = React.createRef();

let formSubmitted = false;

function Form() {
const [active, setActive] = React.useState(true);
function disableForm() {
setActive(false);
}
function submitForm() {
formSubmitted = true; // This should not get invoked
}

return (
<div>
<button onClick={disableForm} ref={disableButtonRef}>
Disable
</button>
{active ? (
<button onClick={submitForm} ref={submitButtonRef}>
Submit
</button>
) : null}
{active ? <button ref={submitButtonRef}>Submit</button> : null}
</div>
);
}

const root = ReactDOM.unstable_createRoot(container);
root.render(<Form />);
// Flush
Scheduler.unstable_flushAll();
await act(async () => {
root.render(<Form />);
});

const disableButton = disableButtonRef.current;
expect(disableButton.tagName).toBe('BUTTON');

const submitButton = submitButtonRef.current;
expect(submitButton.tagName).toBe('BUTTON');

// Dispatch a click event on the Disable-button.
const firstEvent = document.createEvent('Event');
firstEvent.initEvent('click', true, true);
disableButton.dispatchEvent(firstEvent);

// There should now be a pending update to disable the form.

// This should not have flushed yet since it's in concurrent mode.
const submitButton = submitButtonRef.current;
expect(submitButton.tagName).toBe('BUTTON');

// In the meantime, we can dispatch a new client event on the submit button.
const secondEvent = document.createEvent('Event');
secondEvent.initEvent('click', true, true);
// This should force the pending update to flush which disables the submit button before the event is invoked.
submitButton.dispatchEvent(secondEvent);

// Therefore the form should never have been submitted.
expect(formSubmitted).toBe(false);

expect(submitButtonRef.current).toBe(null);
// The click event is flushed synchronously, even in concurrent mode.
expect(submitButton.current).toBe(undefined);
});

// @gate experimental
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
}

const root = ReactDOM.unstable_createRoot(container);
root.render(<Form />);
// Flush
Scheduler.unstable_flushAll();
await act(() => {
root.render(<Form />);
});

const disableButton = disableButtonRef.current;
expect(disableButton.tagName).toBe('BUTTON');
Expand All @@ -81,11 +81,6 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
dispatchAndSetCurrentEvent(disableButton, firstEvent),
).toErrorDev(['An update to Form inside a test was not wrapped in act']);

// There should now be a pending update to disable the form.
// This should not have flushed yet since it's in concurrent mode.
const submitButton = submitButtonRef.current;
expect(submitButton.tagName).toBe('BUTTON');

// Discrete events should be flushed in a microtask.
// Verify that the second button was removed.
await null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ describe('ChangeEventPlugin', () => {
});

// @gate experimental
it('is async for non-input events', async () => {
it('is sync for non-input events', async () => {
const root = ReactDOM.unstable_createRoot(container);
let input;

Expand Down Expand Up @@ -724,9 +724,6 @@ describe('ChangeEventPlugin', () => {
input.dispatchEvent(
new Event('click', {bubbles: true, cancelable: true}),
);
// Nothing should have changed
expect(Scheduler).toHaveYielded([]);
expect(input.value).toBe('initial');

// Flush microtask queue.
await null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ describe('SimpleEventPlugin', function() {
});

// @gate experimental
it('flushes pending interactive work before extracting event handler', () => {
it('flushes pending interactive work before exiting event handler', () => {
container = document.createElement('div');
const root = ReactDOM.unstable_createRoot(container);
document.body.appendChild(container);
Expand Down Expand Up @@ -292,17 +292,14 @@ describe('SimpleEventPlugin', function() {
expect(Scheduler).toHaveYielded([
// The handler fired
'Side-effect',
// but the component did not re-render yet, because it's async
// The component re-rendered synchronously, even in concurrent mode.
'render button: disabled',
]);

// Click the button again
click();
expect(Scheduler).toHaveYielded([
// Before handling this second click event, the previous interactive
// update is flushed
'render button: disabled',
// The event handler was removed from the button, so there's no second
// side-effect
// The event handler was removed from the button, so there's no effect.
]);

// The handler should not fire again no matter how many times we
Expand Down Expand Up @@ -359,8 +356,8 @@ describe('SimpleEventPlugin', function() {

// Click the button a single time
click();
// The counter should not have updated yet because it's async
expect(button.textContent).toEqual('Count: 0');
// The counter should update synchronously, even in concurrent mode.
expect(button.textContent).toEqual('Count: 1');

// Click the button many more times
await TestUtils.act(async () => {
Expand Down Expand Up @@ -442,15 +439,10 @@ describe('SimpleEventPlugin', function() {
button.dispatchEvent(event);
}

// Click the button a single time
click();
// Nothing should flush on the first click.
expect(Scheduler).toHaveYielded([]);
// Click again. This will force the previous discrete update to flush. But
// only the high-pri count will increase.
// Click the button a single time.
// This will flush at the end of the event, even in concurrent mode.
click();
expect(Scheduler).toHaveYielded(['High-pri count: 1, Low-pri count: 0']);
expect(button.textContent).toEqual('High-pri count: 1, Low-pri count: 0');

// Click the button many more times
click();
Expand All @@ -460,7 +452,7 @@ describe('SimpleEventPlugin', function() {
click();
click();

// Flush the remaining work.
// Each update should synchronously flush, even in concurrent mode.
expect(Scheduler).toHaveYielded([
'High-pri count: 2, Low-pri count: 0',
'High-pri count: 3, Low-pri count: 0',
Expand All @@ -470,16 +462,13 @@ describe('SimpleEventPlugin', function() {
'High-pri count: 7, Low-pri count: 0',
]);

// Flush the microtask queue
await null;

// At the end, both counters should equal the total number of clicks
expect(Scheduler).toHaveYielded(['High-pri count: 8, Low-pri count: 0']);
// Now flush the scheduler to apply the transition updates.
// At the end, both counters should equal the total number of clicks.
expect(Scheduler).toFlushAndYield([
'High-pri count: 8, Low-pri count: 8',
'High-pri count: 7, Low-pri count: 7',
]);

expect(button.textContent).toEqual('High-pri count: 8, Low-pri count: 8');
expect(button.textContent).toEqual('High-pri count: 7, Low-pri count: 7');
});
});

Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberLane.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ export function findUpdateLane(lanePriority: LanePriority): Lane {
case SyncBatchedLanePriority:
return SyncBatchedLane;
case InputDiscreteLanePriority:
return InputDiscreteLane;
return SyncLane;
case InputContinuousLanePriority:
return InputContinuousLane;
case DefaultLanePriority:
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberLane.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ export function findUpdateLane(lanePriority: LanePriority): Lane {
case SyncBatchedLanePriority:
return SyncBatchedLane;
case InputDiscreteLanePriority:
return InputDiscreteLane;
return SyncLane;
case InputContinuousLanePriority:
return InputContinuousLane;
case DefaultLanePriority:
Expand Down
7 changes: 0 additions & 7 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ import {
clearContainer,
getCurrentEventPriority,
supportsMicrotasks,
scheduleMicrotask,
} from './ReactFiberHostConfig';

import {
Expand Down Expand Up @@ -737,12 +736,6 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
ImmediateSchedulerPriority,
performSyncWorkOnRoot.bind(null, root),
);
} else if (
supportsMicrotasks &&
newCallbackPriority === InputDiscreteLanePriority
) {
scheduleMicrotask(performSyncWorkOnRoot.bind(null, root));
newCallbackNode = null;
} else {
const schedulerPriorityLevel = lanePriorityToSchedulerPriority(
newCallbackPriority,
Expand Down
7 changes: 0 additions & 7 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ import {
clearContainer,
getCurrentEventPriority,
supportsMicrotasks,
scheduleMicrotask,
} from './ReactFiberHostConfig';

import {
Expand Down Expand Up @@ -737,12 +736,6 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
ImmediateSchedulerPriority,
performSyncWorkOnRoot.bind(null, root),
);
} else if (
supportsMicrotasks &&
newCallbackPriority === InputDiscreteLanePriority
) {
scheduleMicrotask(performSyncWorkOnRoot.bind(null, root));
newCallbackNode = null;
} else {
const schedulerPriorityLevel = lanePriorityToSchedulerPriority(
newCallbackPriority,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,7 @@ describe('SchedulingProfiler labels', () => {
targetRef.current.click();
});
expect(clearedMarks).toContain(
`--schedule-state-update-${formatLanes(
ReactFiberLane.InputDiscreteLane,
)}-App`,
`--schedule-state-update-${formatLanes(ReactFiberLane.SyncLane)}-App`,
);
});

Expand Down

0 comments on commit 81880dc

Please sign in to comment.