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

Re-add discrete flushing timeStamp heuristic (behind flag) #19540

Merged
merged 3 commits into from
Aug 6, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions packages/react-dom/src/__tests__/ReactDOMFiber-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,17 @@ describe('ReactDOMFiber', () => {
const handlerA = () => ops.push('A');
const handlerB = () => ops.push('B');

function click() {
const event = new MouseEvent('click', {
bubbles: true,
cancelable: true,
});
Object.defineProperty(event, 'timeStamp', {
value: 0,
});
node.dispatchEvent(event);
}

class Example extends React.Component {
state = {flip: false, count: 0};
flip() {
Expand Down Expand Up @@ -1064,23 +1075,23 @@ describe('ReactDOMFiber', () => {
const node = container.firstChild;
expect(node.tagName).toEqual('DIV');

node.click();
click();

expect(ops).toEqual(['A']);
ops = [];

// Render with the other event handler.
inst.flip();

node.click();
click();

expect(ops).toEqual(['B']);
ops = [];

// Rerender without changing any props.
inst.tick();

node.click();
click();

expect(ops).toEqual(['B']);
ops = [];
Expand All @@ -1100,7 +1111,7 @@ describe('ReactDOMFiber', () => {
ops = [];

// Any click that happens after commit, should invoke A.
node.click();
click();
expect(ops).toEqual(['A']);
});

Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/events/ReactDOMEventListener.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ function dispatchDiscreteEvent(
// flushed for this event and we don't need to do it again.
(eventSystemFlags & IS_LEGACY_FB_SUPPORT_MODE) === 0
) {
flushDiscreteUpdatesIfNeeded();
flushDiscreteUpdatesIfNeeded(nativeEvent.timeStamp);
}
discreteUpdates(
dispatchEvent,
Expand Down
31 changes: 28 additions & 3 deletions packages/react-dom/src/events/ReactDOMUpdateBatching.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
needsStateRestore,
restoreStateIfNeeded,
} from './ReactDOMControlledComponent';
import {enableDiscreteEventFlushingChange} from 'shared/ReactFeatureFlags';

// Used as a way to call batchedUpdates when we don't have a reference to
// the renderer. Such as when we're dispatching events or if third party
Expand Down Expand Up @@ -87,9 +88,33 @@ export function discreteUpdates(fn, a, b, c, d) {
}
}

export function flushDiscreteUpdatesIfNeeded() {
if (!isInsideEventHandler) {
flushDiscreteUpdatesImpl();
let lastFlushedEventTimeStamp = 0;
export function flushDiscreteUpdatesIfNeeded(timeStamp: number) {
if (enableDiscreteEventFlushingChange) {
// event.timeStamp isn't overly reliable due to inconsistencies in
// how different browsers have historically provided the time stamp.
// Some browsers provide high-resolution time stamps for all events,
// some provide low-resolution time stamps for all events. FF < 52
// even mixes both time stamps together. Some browsers even report
// negative time stamps or time stamps that are 0 (iOS9) in some cases.
// Given we are only comparing two time stamps with equality (!==),
// we are safe from the resolution differences. If the time stamp is 0
// we bail-out of preventing the flush, which can affect semantics,
// such as if an earlier flush removes or adds event listeners that
// are fired in the subsequent flush. However, this is the same
// behaviour as we had before this change, so the risks are low.
if (
!isInsideEventHandler &&
(timeStamp === 0 || lastFlushedEventTimeStamp !== timeStamp)
) {
lastFlushedEventTimeStamp = timeStamp;
flushDiscreteUpdatesImpl();
}
} else {
if (!isInsideEventHandler) {
lastFlushedEventTimeStamp = timeStamp;
trueadm marked this conversation as resolved.
Show resolved Hide resolved
flushDiscreteUpdatesImpl();
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,14 @@ describe('SimpleEventPlugin', function() {
expect(Scheduler).toFlushAndYield(['render button: enabled']);

function click() {
button.dispatchEvent(
new MouseEvent('click', {bubbles: true, cancelable: true}),
);
const event = new MouseEvent('click', {
bubbles: true,
cancelable: true,
});
Object.defineProperty(event, 'timeStamp', {
value: 0,
});
button.dispatchEvent(event);
}

// Click the button to trigger the side-effect
Expand Down Expand Up @@ -340,9 +345,14 @@ describe('SimpleEventPlugin', function() {
expect(button.textContent).toEqual('Count: 0');

function click() {
button.dispatchEvent(
new MouseEvent('click', {bubbles: true, cancelable: true}),
);
const event = new MouseEvent('click', {
bubbles: true,
cancelable: true,
});
Object.defineProperty(event, 'timeStamp', {
value: 0,
});
button.dispatchEvent(event);
}

// Click the button a single time
Expand Down Expand Up @@ -421,9 +431,14 @@ describe('SimpleEventPlugin', function() {
expect(button.textContent).toEqual('High-pri count: 0, Low-pri count: 0');

function click() {
button.dispatchEvent(
new MouseEvent('click', {bubbles: true, cancelable: true}),
);
const event = new MouseEvent('click', {
bubbles: true,
cancelable: true,
});
Object.defineProperty(event, 'timeStamp', {
value: 0,
});
button.dispatchEvent(event);
}

// Click the button a single time
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,3 +125,5 @@ export const deferRenderPhaseUpdateToNextBatch = true;

// Replacement for runWithPriority in React internals.
export const decoupleUpdatePriorityFromScheduler = false;

export const enableDiscreteEventFlushingChange = false;
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = true;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ export const disableTextareaChildren = __EXPERIMENTAL__;

export const warnUnstableRenderSubtreeIntoContainer = false;

export const enableDiscreteEventFlushingChange = true;

// Enable forked reconciler. Piggy-backing on the "variant" global so that we
// don't have to add another test dimension. The build system will compile this
// to the correct value.
Expand Down