From 9488108b4b36467d3b126c57636b83d53aad874d Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Tue, 24 Apr 2018 10:59:56 -0700 Subject: [PATCH 1/8] Add support for multiple callbacks in ReactScheduler **what is the change?:** We want to support calling ReactScheduler multiple times with different callbacks, even if the initial callback hasn't been called yet. There are two possible ways ReactScheduler can handle multiple callbacks, and in one case we know that callbackA depends on callbackB having been called already. For example; callbackA -> updates SelectionState in a textArea callbackB -> processes the deletion of the text currently selected. We want to ensure that callbackA happens before callbackB. For now we will flush callbackB as soon as callbackA is added. In the next commit we'll split this into two methods, which support two different behaviors here. We will support the usual behavior, which would defer both callbackA and callbackB. One issue with this is that we now create a new object to pass to the callback for every use of the scheduler, while before we reused the same object and mutated the 'didExpire' before passing it to each new callback. With multiple callbacks, I think this leads to a risk of mutating the object which is being used by multiple callbacks. **why make this change?:** We want to use this scheduling helper to coordinate between React and non-React scripts. **test plan:** Added and ran a unit test. --- .../react-scheduler/src/ReactScheduler.js | 52 +++++++++++++------ .../src/__tests__/ReactScheduler-test.js | 24 ++++++++- 2 files changed, 58 insertions(+), 18 deletions(-) diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index b15e5f4428e..2555d775e01 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -14,10 +14,13 @@ * control than requestAnimationFrame and requestIdleCallback. * Current TODO items: * X- Pull out the rIC polyfill built into React - * - Initial test coverage - * - Support for multiple callbacks + * X- Initial test coverage + * X- Support for multiple callbacks * - Support for two priorities; serial and deferred * - Better test coverage + * - Mock out the react-scheduler module, not the browser APIs, in renderer + * tests + * - Add headless browser test of react-scheduler * - Better docblock * - Polish documentation, API */ @@ -100,12 +103,20 @@ if (!ExecutionEnvironment.canUseDOM) { let previousFrameTime = 33; let activeFrameTime = 33; - const frameDeadlineObject = { - didTimeout: false, - timeRemaining() { - const remaining = frameDeadline - now(); - return remaining > 0 ? remaining : 0; - }, + const buildFrameDeadlineObject = function(didTimeout: boolean) { + return { + didTimeout, + timeRemaining() { + const remaining = frameDeadline - now(); + return remaining > 0 ? remaining : 0; + }, + }; + }; + + // define a helper for this because they should usually happen together + const clearTimeoutTimeAndScheduledCallback = function() { + timeoutTime = -1; + scheduledRICCallback = null; }; // We use the postMessage trick to defer idle work until after the repaint. @@ -122,13 +133,14 @@ if (!ExecutionEnvironment.canUseDOM) { isIdleScheduled = false; const currentTime = now(); + let didTimeout = false; if (frameDeadline - currentTime <= 0) { // There's no time left in this idle period. Check if the callback has // a timeout and whether it's been exceeded. if (timeoutTime !== -1 && timeoutTime <= currentTime) { // Exceeded the timeout. Invoke the callback even though there's no // time left. - frameDeadlineObject.didTimeout = true; + didTimeout = true; } else { // No timeout. if (!isAnimationFrameScheduled) { @@ -141,14 +153,13 @@ if (!ExecutionEnvironment.canUseDOM) { } } else { // There's still time left in this idle period. - frameDeadlineObject.didTimeout = false; + didTimeout = false; } - timeoutTime = -1; const callback = scheduledRICCallback; - scheduledRICCallback = null; + clearTimeoutTimeAndScheduledCallback(); if (callback !== null) { - callback(frameDeadlineObject); + callback(buildFrameDeadlineObject(didTimeout)); } }; // Assumes that we have addEventListener in this environment. Might need @@ -190,11 +201,19 @@ if (!ExecutionEnvironment.canUseDOM) { callback: (deadline: Deadline) => void, options?: {timeout: number}, ): number { - // This assumes that we only schedule one callback at a time because that's - // how Fiber uses it. + if (scheduledRICCallback !== null) { + // Handling multiple callbacks: + // For now we implement the behavior expected when the callbacks are + // serial updates, such that each update relies on the previous ones + // having been called before it runs. + const didTimeout = (timeoutTime !== -1) && (timeoutTime <= now()); + scheduledRICCallback(buildFrameDeadlineObject(didTimeout)); + } scheduledRICCallback = callback; if (options != null && typeof options.timeout === 'number') { timeoutTime = now() + options.timeout; + } else { + timeoutTime = -1; } if (!isAnimationFrameScheduled) { // If rAF didn't already schedule one, we need to schedule a frame. @@ -208,9 +227,8 @@ if (!ExecutionEnvironment.canUseDOM) { }; cIC = function() { - scheduledRICCallback = null; isIdleScheduled = false; - timeoutTime = -1; + clearTimeoutTimeAndScheduledCallback(); }; } diff --git a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js index 1ed60b0fb31..f1cfc992083 100644 --- a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js +++ b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js @@ -47,10 +47,32 @@ describe('ReactScheduler', () => { rIC(cb); jest.runAllTimers(); expect(cb.mock.calls.length).toBe(1); - // should have ... TODO details on what we expect + // should not have timed out and should include a timeRemaining method expect(cb.mock.calls[0][0].didTimeout).toBe(false); expect(typeof cb.mock.calls[0][0].timeRemaining()).toBe('number'); }); + it('rIC with multiple callbacks flushes previous cb when new one is passed', () => { + const {rIC} = ReactScheduler; + const callbackA = jest.fn(); + const callbackB = jest.fn(); + rIC(callbackA); + // initially waits to call the callback + expect(callbackA.mock.calls.length).toBe(0); + // when second callback is passed, flushes first one + rIC(callbackB); + expect(callbackA.mock.calls.length).toBe(1); + expect(callbackB.mock.calls.length).toBe(0); + // after a delay, calls the latest callback passed + jest.runAllTimers(); + expect(callbackA.mock.calls.length).toBe(1); + expect(callbackB.mock.calls.length).toBe(1); + // callbackA should not have timed out and should include a timeRemaining method + expect(callbackA.mock.calls[0][0].didTimeout).toBe(false); + expect(typeof callbackA.mock.calls[0][0].timeRemaining()).toBe('number'); + // callbackA should not have timed out and should include a timeRemaining method + expect(callbackB.mock.calls[0][0].didTimeout).toBe(false); + expect(typeof callbackB.mock.calls[0][0].timeRemaining()).toBe('number'); + }); // TODO: test cIC and now }); From 5364c9a09f8dc4fe76ada655e74de910be48feb9 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 25 Apr 2018 08:53:37 -0700 Subject: [PATCH 2/8] Move back to using pooling with frameDeadlineObject --- .../react-scheduler/src/ReactScheduler.js | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index 2555d775e01..3976f32fe1f 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -103,14 +103,12 @@ if (!ExecutionEnvironment.canUseDOM) { let previousFrameTime = 33; let activeFrameTime = 33; - const buildFrameDeadlineObject = function(didTimeout: boolean) { - return { - didTimeout, - timeRemaining() { - const remaining = frameDeadline - now(); - return remaining > 0 ? remaining : 0; - }, - }; + const frameDeadlineObject = { + didTimeout: false, + timeRemaining() { + const remaining = frameDeadline - now(); + return remaining > 0 ? remaining : 0; + }, }; // define a helper for this because they should usually happen together @@ -159,7 +157,8 @@ if (!ExecutionEnvironment.canUseDOM) { const callback = scheduledRICCallback; clearTimeoutTimeAndScheduledCallback(); if (callback !== null) { - callback(buildFrameDeadlineObject(didTimeout)); + frameDeadlineObject.didTimeout = didTimeout; + callback(frameDeadlineObject); } }; // Assumes that we have addEventListener in this environment. Might need @@ -206,8 +205,9 @@ if (!ExecutionEnvironment.canUseDOM) { // For now we implement the behavior expected when the callbacks are // serial updates, such that each update relies on the previous ones // having been called before it runs. - const didTimeout = (timeoutTime !== -1) && (timeoutTime <= now()); - scheduledRICCallback(buildFrameDeadlineObject(didTimeout)); + frameDeadlineObject.didTimeout = + (timeoutTime !== -1) && (timeoutTime <= now()); + scheduledRICCallback(frameDeadlineObject); } scheduledRICCallback = callback; if (options != null && typeof options.timeout === 'number') { From 9a1155d5d555199e592877c6815702a51e563797 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 25 Apr 2018 08:56:34 -0700 Subject: [PATCH 3/8] clearTimeoutTimeAndScheduledCallback -> resetScheduledCallback --- packages/react-scheduler/src/ReactScheduler.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index 3976f32fe1f..575bdbc8c7b 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -112,7 +112,7 @@ if (!ExecutionEnvironment.canUseDOM) { }; // define a helper for this because they should usually happen together - const clearTimeoutTimeAndScheduledCallback = function() { + const resetScheduledCallback = function() { timeoutTime = -1; scheduledRICCallback = null; }; @@ -155,7 +155,7 @@ if (!ExecutionEnvironment.canUseDOM) { } const callback = scheduledRICCallback; - clearTimeoutTimeAndScheduledCallback(); + resetScheduledCallback(); if (callback !== null) { frameDeadlineObject.didTimeout = didTimeout; callback(frameDeadlineObject); @@ -228,7 +228,7 @@ if (!ExecutionEnvironment.canUseDOM) { cIC = function() { isIdleScheduled = false; - clearTimeoutTimeAndScheduledCallback(); + resetScheduledCallback(); }; } From 48791d1063c2329a02924f082853bf1a44bebf2f Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 25 Apr 2018 09:02:13 -0700 Subject: [PATCH 4/8] Assert callback order in scheduler test for multiple callback support --- .../src/__tests__/ReactScheduler-test.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js index f1cfc992083..f6429ed9493 100644 --- a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js +++ b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js @@ -54,19 +54,21 @@ describe('ReactScheduler', () => { it('rIC with multiple callbacks flushes previous cb when new one is passed', () => { const {rIC} = ReactScheduler; - const callbackA = jest.fn(); - const callbackB = jest.fn(); + const callbackLog = []; + const callbackA = jest.fn(() => callbackLog.push('A')); + const callbackB = jest.fn(() => callbackLog.push('B')); rIC(callbackA); // initially waits to call the callback - expect(callbackA.mock.calls.length).toBe(0); + expect(callbackLog.length).toBe(0); // when second callback is passed, flushes first one rIC(callbackB); - expect(callbackA.mock.calls.length).toBe(1); - expect(callbackB.mock.calls.length).toBe(0); + expect(callbackLog.length).toBe(1); + expect(callbackLog[0]).toBe('A'); // after a delay, calls the latest callback passed jest.runAllTimers(); - expect(callbackA.mock.calls.length).toBe(1); - expect(callbackB.mock.calls.length).toBe(1); + expect(callbackLog.length).toBe(2); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); // callbackA should not have timed out and should include a timeRemaining method expect(callbackA.mock.calls[0][0].didTimeout).toBe(false); expect(typeof callbackA.mock.calls[0][0].timeRemaining()).toBe('number'); From 4d680070c24dd7ecb142cf4e40c237955bc8c238 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 25 Apr 2018 14:25:48 -0700 Subject: [PATCH 5/8] Fix case of scheduled callbacks which schedule other callbacks **what is the change?:** We added support for serial scheduled callbacks to schedule more callbacks, and maintained the order of first-in first-called. **why make this change?:** This is sort of a corner case, but it's totally possible and we do something similar in React. We wouldn't do this with serial callbacks, but with deferred callbacks we do a pattern like this: ``` + + | +--------------------+ +----------------+ | +--------------------------------+ +-------------------------+ | | | | | | | | | | | | main thread blocked| |callback A runs | | | main thread blocked again | |callback A runs again, finishes | +--------------------+ +-----+----------+ | +--------------------------------+ ++------------------------+ v ^ v ^ schedule +------------------------->+ no time left!+----------------------->+ callbackA reschedule | callbackA + to do more work later. ``` **test plan:** Wrote some fun new tests and ran them~ Also ran existing React unit tests. As of this PR they are still directly using this module. Rename ReactScheduler.rIC -> scheduleSerialCallback **what is the change?:** We renamed this method. **why make this change?:** 1) This is no longer just a polyfill for requestIdleCallback. It's becoming something of it's own. 2) We are about to introduce a second variant of behavior for handling scheduling of callbacks, so want to have names which make sense for differentiating the two priorities. **test plan:** Ran tests --- packages/react-art/src/ReactART.js | 2 +- packages/react-dom/src/client/ReactDOM.js | 2 +- .../react-scheduler/src/ReactScheduler.js | 107 +++++++--- .../src/__tests__/ReactScheduler-test.js | 188 +++++++++++++++--- 4 files changed, 243 insertions(+), 56 deletions(-) diff --git a/packages/react-art/src/ReactART.js b/packages/react-art/src/ReactART.js index 04478434e8b..e1e9ac8563c 100644 --- a/packages/react-art/src/ReactART.js +++ b/packages/react-art/src/ReactART.js @@ -468,7 +468,7 @@ const ARTRenderer = ReactFiberReconciler({ return emptyObject; }, - scheduleDeferredCallback: ReactScheduler.rIC, + scheduleDeferredCallback: ReactScheduler.scheduleSerialCallback, shouldSetTextContent(type, props) { return ( diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 82032f1d1d2..c61a1d6d4f7 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -984,7 +984,7 @@ const DOMRenderer = ReactFiberReconciler({ }, }, - scheduleDeferredCallback: ReactScheduler.rIC, + scheduleDeferredCallback: ReactScheduler.scheduleSerialCallback, cancelDeferredCallback: ReactScheduler.cIC, }); diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index 575bdbc8c7b..ee65a6216c6 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -16,11 +16,11 @@ * X- Pull out the rIC polyfill built into React * X- Initial test coverage * X- Support for multiple callbacks - * - Support for two priorities; serial and deferred + * X- Support for two priorities; serial and deferred * - Better test coverage * - Mock out the react-scheduler module, not the browser APIs, in renderer * tests - * - Add headless browser test of react-scheduler + * - Add fixture test of react-scheduler * - Better docblock * - Polish documentation, API */ @@ -66,13 +66,13 @@ if (hasNativePerformanceNow) { } // TODO: There's no way to cancel, because Fiber doesn't atm. -let rIC: ( +let scheduleSerialCallback: ( callback: (deadline: Deadline, options?: {timeout: number}) => void, ) => number; let cIC: (callbackID: number) => void; if (!ExecutionEnvironment.canUseDOM) { - rIC = function( + scheduleSerialCallback = function( frameCallback: (deadline: Deadline, options?: {timeout: number}) => void, ): number { return setTimeout(() => { @@ -90,9 +90,12 @@ if (!ExecutionEnvironment.canUseDOM) { } else { // Always polyfill requestIdleCallback and cancelIdleCallback - let scheduledRICCallback = null; + let scheduledCallback = null; let isIdleScheduled = false; let timeoutTime = -1; + let isCurrentlyRunningCallback = false; + // We may need to keep queues of pending callbacks + let pendingSerialCallbacks = []; let isAnimationFrameScheduled = false; @@ -111,12 +114,6 @@ if (!ExecutionEnvironment.canUseDOM) { }, }; - // define a helper for this because they should usually happen together - const resetScheduledCallback = function() { - timeoutTime = -1; - scheduledRICCallback = null; - }; - // We use the postMessage trick to defer idle work until after the repaint. const messageKey = '__reactIdleCallback$' + @@ -154,11 +151,14 @@ if (!ExecutionEnvironment.canUseDOM) { didTimeout = false; } - const callback = scheduledRICCallback; - resetScheduledCallback(); + const callback = scheduledCallback; + timeoutTime = -1; + scheduledCallback = null; if (callback !== null) { frameDeadlineObject.didTimeout = didTimeout; + isCurrentlyRunningCallback = true; callback(frameDeadlineObject); + isCurrentlyRunningCallback = false; } }; // Assumes that we have addEventListener in this environment. Might need @@ -196,40 +196,91 @@ if (!ExecutionEnvironment.canUseDOM) { } }; - rIC = function( + /** + * 'Serial' callbacks are distinct from regular callbacks because they rely on + * all previous 'serial' callbacks having been evaluated. + * For example: If I click 'submit' and then quickly click 'submit' again. The + * first click should disable the 'submit' button, and we can't process the + * second click until that first click has been processed. + */ + scheduleSerialCallback = function( callback: (deadline: Deadline) => void, options?: {timeout: number}, ): number { - if (scheduledRICCallback !== null) { - // Handling multiple callbacks: - // For now we implement the behavior expected when the callbacks are - // serial updates, such that each update relies on the previous ones - // having been called before it runs. - frameDeadlineObject.didTimeout = - (timeoutTime !== -1) && (timeoutTime <= now()); - scheduledRICCallback(frameDeadlineObject); + // Handling multiple callbacks: + // For now we implement the behavior expected when the callbacks are + // serial updates, such that each update relies on the previous ones + // having been called before it runs. + // So we call anything in the queue before the latest callback + + let previousCallback; + let timeoutTimeFromPreviousCallback; + if (scheduledCallback !== null) { + // If we have previous callback, save it and handle it below + timeoutTimeFromPreviousCallback = timeoutTime; + previousCallback = scheduledCallback; } - scheduledRICCallback = callback; + // Then set up the next callback, and update timeoutTime + scheduledCallback = callback; if (options != null && typeof options.timeout === 'number') { timeoutTime = now() + options.timeout; } else { timeoutTime = -1; } + // If we have previousCallback, call it. This may trigger recursion. + if ( + previousCallback && + typeof timeoutTimeFromPreviousCallback === 'number' + ) { + const prevCallbackTimeout: number = timeoutTimeFromPreviousCallback; + if (isCurrentlyRunningCallback) { + // we are inside a recursive call to scheduleSerialCallback + // add this callback to a pending queue and run after we exit + pendingSerialCallbacks.push({ + pendingCallback: previousCallback, + pendingCallbackTimeout: prevCallbackTimeout, + }); + } else { + frameDeadlineObject.didTimeout = + timeoutTimeFromPreviousCallback !== -1 && + timeoutTimeFromPreviousCallback <= now(); + isCurrentlyRunningCallback = true; + previousCallback(frameDeadlineObject); + isCurrentlyRunningCallback = false; + while (pendingSerialCallbacks.length) { + // the callback recursively called scheduleSerialCallback + // and new callbacks are pending + const { + pendingCallback, + pendingCallbackTimeout, + } = pendingSerialCallbacks.shift(); + // TODO: pull this into helper method + frameDeadlineObject.didTimeout = + pendingCallbackTimeout !== -1 && pendingCallbackTimeout <= now(); + isCurrentlyRunningCallback = true; + pendingCallback(frameDeadlineObject); + isCurrentlyRunningCallback = false; + } + } + } + + // finally, after clearing previous callbacks, schedule the latest one if (!isAnimationFrameScheduled) { // If rAF didn't already schedule one, we need to schedule a frame. // TODO: If this rAF doesn't materialize because the browser throttles, we - // might want to still have setTimeout trigger rIC as a backup to ensure - // that we keep performing work. + // might want to still have setTimeout trigger scheduleSerialCallback as a + // backup to ensure that we keep performing work. isAnimationFrameScheduled = true; - requestAnimationFrame(animationTick); + return requestAnimationFrame(animationTick); } return 0; }; cIC = function() { isIdleScheduled = false; - resetScheduledCallback(); + scheduledCallback = null; + timeoutTime = -1; }; } -export {now, rIC, cIC}; +export {now, scheduleSerialCallback, cIC}; diff --git a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js index f6429ed9493..f2cec317f89 100644 --- a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js +++ b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js @@ -41,10 +41,10 @@ describe('ReactScheduler', () => { ReactScheduler = require('react-scheduler'); }); - it('rIC calls the callback within the frame when not blocked', () => { - const {rIC} = ReactScheduler; + it('scheduleSerialCallback calls the callback within the frame when not blocked', () => { + const {scheduleSerialCallback} = ReactScheduler; const cb = jest.fn(); - rIC(cb); + scheduleSerialCallback(cb); jest.runAllTimers(); expect(cb.mock.calls.length).toBe(1); // should not have timed out and should include a timeRemaining method @@ -52,29 +52,165 @@ describe('ReactScheduler', () => { expect(typeof cb.mock.calls[0][0].timeRemaining()).toBe('number'); }); - it('rIC with multiple callbacks flushes previous cb when new one is passed', () => { - const {rIC} = ReactScheduler; - const callbackLog = []; - const callbackA = jest.fn(() => callbackLog.push('A')); - const callbackB = jest.fn(() => callbackLog.push('B')); - rIC(callbackA); - // initially waits to call the callback - expect(callbackLog.length).toBe(0); - // when second callback is passed, flushes first one - rIC(callbackB); - expect(callbackLog.length).toBe(1); - expect(callbackLog[0]).toBe('A'); - // after a delay, calls the latest callback passed - jest.runAllTimers(); - expect(callbackLog.length).toBe(2); - expect(callbackLog[0]).toBe('A'); - expect(callbackLog[1]).toBe('B'); - // callbackA should not have timed out and should include a timeRemaining method - expect(callbackA.mock.calls[0][0].didTimeout).toBe(false); - expect(typeof callbackA.mock.calls[0][0].timeRemaining()).toBe('number'); - // callbackA should not have timed out and should include a timeRemaining method - expect(callbackB.mock.calls[0][0].didTimeout).toBe(false); - expect(typeof callbackB.mock.calls[0][0].timeRemaining()).toBe('number'); + describe('with multiple callbacks', () => { + it('flushes previous cb when new one is passed', () => { + const {scheduleSerialCallback} = ReactScheduler; + const callbackLog = []; + const callbackA = jest.fn(() => callbackLog.push('A')); + const callbackB = jest.fn(() => callbackLog.push('B')); + scheduleSerialCallback(callbackA); + // initially waits to call the callback + expect(callbackLog.length).toBe(0); + // when second callback is passed, flushes first one + scheduleSerialCallback(callbackB); + expect(callbackLog.length).toBe(1); + expect(callbackLog[0]).toBe('A'); + // after a delay, calls the latest callback passed + jest.runAllTimers(); + expect(callbackLog.length).toBe(2); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + // callbackA should not have timed out and should include a timeRemaining method + expect(callbackA.mock.calls[0][0].didTimeout).toBe(false); + expect(typeof callbackA.mock.calls[0][0].timeRemaining()).toBe('number'); + // callbackA should not have timed out and should include a timeRemaining method + expect(callbackB.mock.calls[0][0].didTimeout).toBe(false); + expect(typeof callbackB.mock.calls[0][0].timeRemaining()).toBe('number'); + }); + + it('schedules callbacks in correct order when a callback uses scheduleSerialCallback before its own logic', () => { + const {scheduleSerialCallback} = ReactScheduler; + const callbackLog = []; + const callbackA = jest.fn(() => { + callbackLog.push('A'); + scheduleSerialCallback(callbackC); + }); + const callbackB = jest.fn(() => { + callbackLog.push('B'); + }); + const callbackC = jest.fn(() => { + callbackLog.push('C'); + }); + + scheduleSerialCallback(callbackA); + // initially waits to call the callback + expect(callbackLog.length).toBe(0); + // when second callback is passed, flushes first one + // callbackA scheduled callbackC, which flushes callbackB + scheduleSerialCallback(callbackB); + expect(callbackLog.length).toBe(2); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + // after a delay, calls the latest callback passed + jest.runAllTimers(); + expect(callbackLog.length).toBe(3); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + expect(callbackLog[2]).toBe('C'); + }); + + it('schedules callbacks in correct order when callbacks have many nested scheduleSerialCallback calls', () => { + const {scheduleSerialCallback} = ReactScheduler; + const callbackLog = []; + const callbackA = jest.fn(() => { + callbackLog.push('A'); + scheduleSerialCallback(callbackC); + scheduleSerialCallback(callbackD); + }); + const callbackB = jest.fn(() => { + callbackLog.push('B'); + scheduleSerialCallback(callbackE); + scheduleSerialCallback(callbackF); + }); + const callbackC = jest.fn(() => { + callbackLog.push('C'); + }); + const callbackD = jest.fn(() => { + callbackLog.push('D'); + }); + const callbackE = jest.fn(() => { + callbackLog.push('E'); + }); + const callbackF = jest.fn(() => { + callbackLog.push('F'); + }); + + scheduleSerialCallback(callbackA); + // initially waits to call the callback + expect(callbackLog.length).toBe(0); + // when second callback is passed, flushes first one + // callbackA scheduled callbackC, which flushes callbackB + scheduleSerialCallback(callbackB); + expect(callbackLog.length).toBe(5); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + expect(callbackLog[2]).toBe('C'); + expect(callbackLog[3]).toBe('D'); + expect(callbackLog[4]).toBe('E'); + // after a delay, calls the latest callback passed + jest.runAllTimers(); + expect(callbackLog.length).toBe(6); + expect(callbackLog[5]).toBe('F'); + }); + + it('allows each callback finish running before flushing others', () => { + const {scheduleSerialCallback} = ReactScheduler; + const callbackLog = []; + const callbackA = jest.fn(() => { + // scheduleSerialCallback should wait to flush any more until this callback finishes + scheduleSerialCallback(callbackC); + callbackLog.push('A'); + }); + const callbackB = jest.fn(() => callbackLog.push('B')); + const callbackC = jest.fn(() => callbackLog.push('C')); + + scheduleSerialCallback(callbackA); + // initially waits to call the callback + expect(callbackLog.length).toBe(0); + // when second callback is passed, flushes first one + // callbackA scheduled callbackC, which flushes callbackB + scheduleSerialCallback(callbackB); + expect(callbackLog.length).toBe(2); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + // after a delay, calls the latest callback passed + jest.runAllTimers(); + expect(callbackLog.length).toBe(3); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + expect(callbackLog[2]).toBe('C'); + }); + + it('schedules callbacks in correct order when they use scheduleSerialCallback to schedule themselves', () => { + const {scheduleSerialCallback} = ReactScheduler; + const callbackLog = []; + let callbackAIterations = 0; + const callbackA = jest.fn(() => { + if (callbackAIterations < 1) { + scheduleSerialCallback(callbackA); + } + callbackLog.push('A' + callbackAIterations); + callbackAIterations++; + }); + const callbackB = jest.fn(() => callbackLog.push('B')); + + scheduleSerialCallback(callbackA); + // initially waits to call the callback + expect(callbackLog.length).toBe(0); + // when second callback is passed, flushes first one + // callbackA scheduled callbackA again, which flushes callbackB + scheduleSerialCallback(callbackB); + expect(callbackLog.length).toBe(2); + expect(callbackLog[0]).toBe('A0'); + expect(callbackLog[1]).toBe('B'); + // after a delay, calls the latest callback passed + jest.runAllTimers(); + expect(callbackLog.length).toBe(3); + expect(callbackLog[0]).toBe('A0'); + expect(callbackLog[1]).toBe('B'); + expect(callbackLog[2]).toBe('A1'); + }); }); + // TODO: test cIC and now }); From c0f4f18f269d74cc2a4a4cae448cbb8510087543 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Fri, 27 Apr 2018 10:43:26 -0700 Subject: [PATCH 6/8] Rename cIC -> cancelSerialCallback and add tests for this method **what is the change?:** Rename a method and add tests **why make this change?:** This will set things up for adding a new method which cancels callbacks which have 'deferred' priority. **test plan:** Run the updated tests. --- packages/react-dom/src/client/ReactDOM.js | 2 +- .../react-scheduler/src/ReactScheduler.js | 8 ++--- .../src/__tests__/ReactScheduler-test.js | 32 +++++++++++++++++-- 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index c61a1d6d4f7..8be7e4ef103 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -985,7 +985,7 @@ const DOMRenderer = ReactFiberReconciler({ }, scheduleDeferredCallback: ReactScheduler.scheduleSerialCallback, - cancelDeferredCallback: ReactScheduler.cIC, + cancelDeferredCallback: ReactScheduler.cancelSerialCallback, }); ReactGenericBatching.injection.injectRenderer(DOMRenderer); diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index ee65a6216c6..90ffee8d167 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -69,7 +69,7 @@ if (hasNativePerformanceNow) { let scheduleSerialCallback: ( callback: (deadline: Deadline, options?: {timeout: number}) => void, ) => number; -let cIC: (callbackID: number) => void; +let cancelSerialCallback: (callbackID: number) => void; if (!ExecutionEnvironment.canUseDOM) { scheduleSerialCallback = function( @@ -84,7 +84,7 @@ if (!ExecutionEnvironment.canUseDOM) { }); }); }; - cIC = function(timeoutID: number) { + cancelSerialCallback = function(timeoutID: number) { clearTimeout(timeoutID); }; } else { @@ -276,11 +276,11 @@ if (!ExecutionEnvironment.canUseDOM) { return 0; }; - cIC = function() { + cancelSerialCallback = function() { isIdleScheduled = false; scheduledCallback = null; timeoutTime = -1; }; } -export {now, scheduleSerialCallback, cIC}; +export {now, scheduleSerialCallback, cancelSerialCallback}; diff --git a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js index f2cec317f89..20a0b144d45 100644 --- a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js +++ b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js @@ -41,7 +41,7 @@ describe('ReactScheduler', () => { ReactScheduler = require('react-scheduler'); }); - it('scheduleSerialCallback calls the callback within the frame when not blocked', () => { + it('calls the callback within the frame when not blocked', () => { const {scheduleSerialCallback} = ReactScheduler; const cb = jest.fn(); scheduleSerialCallback(cb); @@ -212,5 +212,33 @@ describe('ReactScheduler', () => { }); }); - // TODO: test cIC and now + describe('cancelSerialCallback', () => { + it('cancels the scheduled callback', () => { + const {scheduleSerialCallback, cancelSerialCallback} = ReactScheduler; + const cb = jest.fn(); + scheduleSerialCallback(cb); + expect(cb.mock.calls.length).toBe(0); + cancelSerialCallback(); + jest.runAllTimers(); + expect(cb.mock.calls.length).toBe(0); + }); + + it('when one callback cancels the next one', () => { + const {scheduleSerialCallback, cancelSerialCallback} = ReactScheduler; + const cbA = jest.fn(() => { + cancelSerialCallback(); + }); + const cbB = jest.fn(); + scheduleSerialCallback(cbA); + expect(cbA.mock.calls.length).toBe(0); + scheduleSerialCallback(cbB); + expect(cbA.mock.calls.length).toBe(1); + expect(cbB.mock.calls.length).toBe(0); + jest.runAllTimers(); + // B should not get called because A cancelled B + expect(cbB.mock.calls.length).toBe(0); + }); + }); + + // TODO: test schedule.now }); From b0a966e55588d0ec3108ba5d243b7f928790055a Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Fri, 27 Apr 2018 10:48:18 -0700 Subject: [PATCH 7/8] Wrap 'scheduleSerialCallback' test in a 'describe' block **what is the change?:** see title **why make this change?:** For more readable and organized test and test output. I put this in it's own commit because the diff looks gnarly, but we're just moving things around. No actual changes. **test plan:** Run the tests --- .../src/__tests__/ReactScheduler-test.js | 318 +++++++++--------- 1 file changed, 162 insertions(+), 156 deletions(-) diff --git a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js index 20a0b144d45..cd96ff11395 100644 --- a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js +++ b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js @@ -41,174 +41,180 @@ describe('ReactScheduler', () => { ReactScheduler = require('react-scheduler'); }); - it('calls the callback within the frame when not blocked', () => { - const {scheduleSerialCallback} = ReactScheduler; - const cb = jest.fn(); - scheduleSerialCallback(cb); - jest.runAllTimers(); - expect(cb.mock.calls.length).toBe(1); - // should not have timed out and should include a timeRemaining method - expect(cb.mock.calls[0][0].didTimeout).toBe(false); - expect(typeof cb.mock.calls[0][0].timeRemaining()).toBe('number'); - }); - - describe('with multiple callbacks', () => { - it('flushes previous cb when new one is passed', () => { + describe('scheduleSerialCallback', () => { + it('calls the callback within the frame when not blocked', () => { const {scheduleSerialCallback} = ReactScheduler; - const callbackLog = []; - const callbackA = jest.fn(() => callbackLog.push('A')); - const callbackB = jest.fn(() => callbackLog.push('B')); - scheduleSerialCallback(callbackA); - // initially waits to call the callback - expect(callbackLog.length).toBe(0); - // when second callback is passed, flushes first one - scheduleSerialCallback(callbackB); - expect(callbackLog.length).toBe(1); - expect(callbackLog[0]).toBe('A'); - // after a delay, calls the latest callback passed + const cb = jest.fn(); + scheduleSerialCallback(cb); jest.runAllTimers(); - expect(callbackLog.length).toBe(2); - expect(callbackLog[0]).toBe('A'); - expect(callbackLog[1]).toBe('B'); - // callbackA should not have timed out and should include a timeRemaining method - expect(callbackA.mock.calls[0][0].didTimeout).toBe(false); - expect(typeof callbackA.mock.calls[0][0].timeRemaining()).toBe('number'); - // callbackA should not have timed out and should include a timeRemaining method - expect(callbackB.mock.calls[0][0].didTimeout).toBe(false); - expect(typeof callbackB.mock.calls[0][0].timeRemaining()).toBe('number'); + expect(cb.mock.calls.length).toBe(1); + // should not have timed out and should include a timeRemaining method + expect(cb.mock.calls[0][0].didTimeout).toBe(false); + expect(typeof cb.mock.calls[0][0].timeRemaining()).toBe('number'); }); - it('schedules callbacks in correct order when a callback uses scheduleSerialCallback before its own logic', () => { - const {scheduleSerialCallback} = ReactScheduler; - const callbackLog = []; - const callbackA = jest.fn(() => { - callbackLog.push('A'); - scheduleSerialCallback(callbackC); - }); - const callbackB = jest.fn(() => { - callbackLog.push('B'); + describe('with multiple callbacks', () => { + it('flushes previous cb when new one is passed', () => { + const {scheduleSerialCallback} = ReactScheduler; + const callbackLog = []; + const callbackA = jest.fn(() => callbackLog.push('A')); + const callbackB = jest.fn(() => callbackLog.push('B')); + scheduleSerialCallback(callbackA); + // initially waits to call the callback + expect(callbackLog.length).toBe(0); + // when second callback is passed, flushes first one + scheduleSerialCallback(callbackB); + expect(callbackLog.length).toBe(1); + expect(callbackLog[0]).toBe('A'); + // after a delay, calls the latest callback passed + jest.runAllTimers(); + expect(callbackLog.length).toBe(2); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + // callbackA should not have timed out and should include a timeRemaining method + expect(callbackA.mock.calls[0][0].didTimeout).toBe(false); + expect(typeof callbackA.mock.calls[0][0].timeRemaining()).toBe( + 'number', + ); + // callbackA should not have timed out and should include a timeRemaining method + expect(callbackB.mock.calls[0][0].didTimeout).toBe(false); + expect(typeof callbackB.mock.calls[0][0].timeRemaining()).toBe( + 'number', + ); }); - const callbackC = jest.fn(() => { - callbackLog.push('C'); - }); - - scheduleSerialCallback(callbackA); - // initially waits to call the callback - expect(callbackLog.length).toBe(0); - // when second callback is passed, flushes first one - // callbackA scheduled callbackC, which flushes callbackB - scheduleSerialCallback(callbackB); - expect(callbackLog.length).toBe(2); - expect(callbackLog[0]).toBe('A'); - expect(callbackLog[1]).toBe('B'); - // after a delay, calls the latest callback passed - jest.runAllTimers(); - expect(callbackLog.length).toBe(3); - expect(callbackLog[0]).toBe('A'); - expect(callbackLog[1]).toBe('B'); - expect(callbackLog[2]).toBe('C'); - }); - it('schedules callbacks in correct order when callbacks have many nested scheduleSerialCallback calls', () => { - const {scheduleSerialCallback} = ReactScheduler; - const callbackLog = []; - const callbackA = jest.fn(() => { - callbackLog.push('A'); - scheduleSerialCallback(callbackC); - scheduleSerialCallback(callbackD); - }); - const callbackB = jest.fn(() => { - callbackLog.push('B'); - scheduleSerialCallback(callbackE); - scheduleSerialCallback(callbackF); - }); - const callbackC = jest.fn(() => { - callbackLog.push('C'); - }); - const callbackD = jest.fn(() => { - callbackLog.push('D'); - }); - const callbackE = jest.fn(() => { - callbackLog.push('E'); - }); - const callbackF = jest.fn(() => { - callbackLog.push('F'); + it('schedules callbacks in correct order when a callback uses scheduleSerialCallback before its own logic', () => { + const {scheduleSerialCallback} = ReactScheduler; + const callbackLog = []; + const callbackA = jest.fn(() => { + callbackLog.push('A'); + scheduleSerialCallback(callbackC); + }); + const callbackB = jest.fn(() => { + callbackLog.push('B'); + }); + const callbackC = jest.fn(() => { + callbackLog.push('C'); + }); + + scheduleSerialCallback(callbackA); + // initially waits to call the callback + expect(callbackLog.length).toBe(0); + // when second callback is passed, flushes first one + // callbackA scheduled callbackC, which flushes callbackB + scheduleSerialCallback(callbackB); + expect(callbackLog.length).toBe(2); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + // after a delay, calls the latest callback passed + jest.runAllTimers(); + expect(callbackLog.length).toBe(3); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + expect(callbackLog[2]).toBe('C'); }); - scheduleSerialCallback(callbackA); - // initially waits to call the callback - expect(callbackLog.length).toBe(0); - // when second callback is passed, flushes first one - // callbackA scheduled callbackC, which flushes callbackB - scheduleSerialCallback(callbackB); - expect(callbackLog.length).toBe(5); - expect(callbackLog[0]).toBe('A'); - expect(callbackLog[1]).toBe('B'); - expect(callbackLog[2]).toBe('C'); - expect(callbackLog[3]).toBe('D'); - expect(callbackLog[4]).toBe('E'); - // after a delay, calls the latest callback passed - jest.runAllTimers(); - expect(callbackLog.length).toBe(6); - expect(callbackLog[5]).toBe('F'); - }); + it('schedules callbacks in correct order when callbacks have many nested scheduleSerialCallback calls', () => { + const {scheduleSerialCallback} = ReactScheduler; + const callbackLog = []; + const callbackA = jest.fn(() => { + callbackLog.push('A'); + scheduleSerialCallback(callbackC); + scheduleSerialCallback(callbackD); + }); + const callbackB = jest.fn(() => { + callbackLog.push('B'); + scheduleSerialCallback(callbackE); + scheduleSerialCallback(callbackF); + }); + const callbackC = jest.fn(() => { + callbackLog.push('C'); + }); + const callbackD = jest.fn(() => { + callbackLog.push('D'); + }); + const callbackE = jest.fn(() => { + callbackLog.push('E'); + }); + const callbackF = jest.fn(() => { + callbackLog.push('F'); + }); + + scheduleSerialCallback(callbackA); + // initially waits to call the callback + expect(callbackLog.length).toBe(0); + // when second callback is passed, flushes first one + // callbackA scheduled callbackC, which flushes callbackB + scheduleSerialCallback(callbackB); + expect(callbackLog.length).toBe(5); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + expect(callbackLog[2]).toBe('C'); + expect(callbackLog[3]).toBe('D'); + expect(callbackLog[4]).toBe('E'); + // after a delay, calls the latest callback passed + jest.runAllTimers(); + expect(callbackLog.length).toBe(6); + expect(callbackLog[5]).toBe('F'); + }); - it('allows each callback finish running before flushing others', () => { - const {scheduleSerialCallback} = ReactScheduler; - const callbackLog = []; - const callbackA = jest.fn(() => { - // scheduleSerialCallback should wait to flush any more until this callback finishes - scheduleSerialCallback(callbackC); - callbackLog.push('A'); + it('allows each callback finish running before flushing others', () => { + const {scheduleSerialCallback} = ReactScheduler; + const callbackLog = []; + const callbackA = jest.fn(() => { + // scheduleSerialCallback should wait to flush any more until this callback finishes + scheduleSerialCallback(callbackC); + callbackLog.push('A'); + }); + const callbackB = jest.fn(() => callbackLog.push('B')); + const callbackC = jest.fn(() => callbackLog.push('C')); + + scheduleSerialCallback(callbackA); + // initially waits to call the callback + expect(callbackLog.length).toBe(0); + // when second callback is passed, flushes first one + // callbackA scheduled callbackC, which flushes callbackB + scheduleSerialCallback(callbackB); + expect(callbackLog.length).toBe(2); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + // after a delay, calls the latest callback passed + jest.runAllTimers(); + expect(callbackLog.length).toBe(3); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + expect(callbackLog[2]).toBe('C'); }); - const callbackB = jest.fn(() => callbackLog.push('B')); - const callbackC = jest.fn(() => callbackLog.push('C')); - - scheduleSerialCallback(callbackA); - // initially waits to call the callback - expect(callbackLog.length).toBe(0); - // when second callback is passed, flushes first one - // callbackA scheduled callbackC, which flushes callbackB - scheduleSerialCallback(callbackB); - expect(callbackLog.length).toBe(2); - expect(callbackLog[0]).toBe('A'); - expect(callbackLog[1]).toBe('B'); - // after a delay, calls the latest callback passed - jest.runAllTimers(); - expect(callbackLog.length).toBe(3); - expect(callbackLog[0]).toBe('A'); - expect(callbackLog[1]).toBe('B'); - expect(callbackLog[2]).toBe('C'); - }); - it('schedules callbacks in correct order when they use scheduleSerialCallback to schedule themselves', () => { - const {scheduleSerialCallback} = ReactScheduler; - const callbackLog = []; - let callbackAIterations = 0; - const callbackA = jest.fn(() => { - if (callbackAIterations < 1) { - scheduleSerialCallback(callbackA); - } - callbackLog.push('A' + callbackAIterations); - callbackAIterations++; + it('schedules callbacks in correct order when they use scheduleSerialCallback to schedule themselves', () => { + const {scheduleSerialCallback} = ReactScheduler; + const callbackLog = []; + let callbackAIterations = 0; + const callbackA = jest.fn(() => { + if (callbackAIterations < 1) { + scheduleSerialCallback(callbackA); + } + callbackLog.push('A' + callbackAIterations); + callbackAIterations++; + }); + const callbackB = jest.fn(() => callbackLog.push('B')); + + scheduleSerialCallback(callbackA); + // initially waits to call the callback + expect(callbackLog.length).toBe(0); + // when second callback is passed, flushes first one + // callbackA scheduled callbackA again, which flushes callbackB + scheduleSerialCallback(callbackB); + expect(callbackLog.length).toBe(2); + expect(callbackLog[0]).toBe('A0'); + expect(callbackLog[1]).toBe('B'); + // after a delay, calls the latest callback passed + jest.runAllTimers(); + expect(callbackLog.length).toBe(3); + expect(callbackLog[0]).toBe('A0'); + expect(callbackLog[1]).toBe('B'); + expect(callbackLog[2]).toBe('A1'); }); - const callbackB = jest.fn(() => callbackLog.push('B')); - - scheduleSerialCallback(callbackA); - // initially waits to call the callback - expect(callbackLog.length).toBe(0); - // when second callback is passed, flushes first one - // callbackA scheduled callbackA again, which flushes callbackB - scheduleSerialCallback(callbackB); - expect(callbackLog.length).toBe(2); - expect(callbackLog[0]).toBe('A0'); - expect(callbackLog[1]).toBe('B'); - // after a delay, calls the latest callback passed - jest.runAllTimers(); - expect(callbackLog.length).toBe(3); - expect(callbackLog[0]).toBe('A0'); - expect(callbackLog[1]).toBe('B'); - expect(callbackLog[2]).toBe('A1'); }); }); From a13a126f48c8ceec1db1a4ceaaddc0654ed8b87e Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Fri, 27 Apr 2018 10:01:31 -0700 Subject: [PATCH 8/8] RFC: Add methods for scheduling **what is the change?:** Add support for multiple priorities. - We are storing pending deferred callbacks in an array for simplicity, which means that we have to do a linear search of the array repeatedly to find callbacks which are past their timeout. It would be more efficient to use a min heap to store them and then quickly find the expired ones, but I'm not sure it's worth the extra complexity unless we know that we'll be dealing with a high number of pending deferred callbacks at once. If there are a high number of pending deferred callbacks, we have other problems to consider. Also the implementation of cancel is missing and I haven't added support for returning ids which can be used to cancel. Not sure we need this yet, but will have to add it eventually. **why make this change?:** To get feedback on this approach as early as possible. **test plan:** Tests are a challenge. To write a unit test I'll need to add support for injecting some of the browser APIs as dependencies, and basically allow us to create a noopScheduler. We also should have a real browser fixture to test this. Going to work on that in another branch. **issue:** Internal task T28128480 --- .../react-scheduler/src/ReactScheduler.js | 169 +++++++++++++----- 1 file changed, 127 insertions(+), 42 deletions(-) diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index 90ffee8d167..175e1e47985 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -69,7 +69,11 @@ if (hasNativePerformanceNow) { let scheduleSerialCallback: ( callback: (deadline: Deadline, options?: {timeout: number}) => void, ) => number; +let scheduleDeferredCallback: ( + callback: (deadline: Deadline, options?: {timeout: number}) => void, +) => number; let cancelSerialCallback: (callbackID: number) => void; +let cancelDeferredCallback: (callback: Function) => void; if (!ExecutionEnvironment.canUseDOM) { scheduleSerialCallback = function( @@ -90,12 +94,13 @@ if (!ExecutionEnvironment.canUseDOM) { } else { // Always polyfill requestIdleCallback and cancelIdleCallback - let scheduledCallback = null; + let scheduledSerialCallback = null; let isIdleScheduled = false; let timeoutTime = -1; let isCurrentlyRunningCallback = false; // We may need to keep queues of pending callbacks let pendingSerialCallbacks = []; + let pendingDeferredCallbacks = []; let isAnimationFrameScheduled = false; @@ -114,6 +119,59 @@ if (!ExecutionEnvironment.canUseDOM) { }, }; + /** + * Checks for timed out callbacks, runs them, and then checks again to see if + * any more have timed out. + * Keeps doing this until there are none which have currently timed out. + */ + const callTimedOutCallbacks = function() { + // TODO: this would be more efficient if deferred callbacks are stored in + // min heap. + let foundTimedOutCallback = false; + + // keep checking until we don't find any more timed out callbacks + do { + const currentTime = now(); + foundTimedOutCallback = false; + // run serial callback if it has timed out + if (scheduledSerialCallback !== null) { + if (timeoutTime !== -1 && timeoutTime <= currentTime) { + foundTimedOutCallback = true; + const currentCallback = scheduledSerialCallback; + timeoutTime = -1; + scheduledSerialCallback = null; + frameDeadlineObject.didTimeout = true; + isCurrentlyRunningCallback = true; + currentCallback(frameDeadlineObject); + isCurrentlyRunningCallback = false; + } + } + if (pendingDeferredCallbacks.length > 0) { + // check if any have timed out, and if so + // run them and remove from pendingDeferredCallbacks + for (let i = 0, len = pendingDeferredCallbacks.length; i < len; i++) { + const { + deferredCallback, + deferredCallbackTimeoutTime, + } = pendingDeferredCallbacks[i]; + if ( + deferredCallbackTimeoutTime !== -1 && + deferredCallbackTimeoutTime <= currentTime + ) { + foundTimedOutCallback = true; + pendingDeferredCallbacks.splice(i, 1); // remove this callback + i--; + len--; // compensate for mutating array we are traversing + frameDeadlineObject.didTimeout = true; + isCurrentlyRunningCallback = true; + deferredCallback(frameDeadlineObject); + isCurrentlyRunningCallback = false; + } + } + } + } while (foundTimedOutCallback); + }; + // We use the postMessage trick to defer idle work until after the repaint. const messageKey = '__reactIdleCallback$' + @@ -127,40 +185,45 @@ if (!ExecutionEnvironment.canUseDOM) { isIdleScheduled = false; - const currentTime = now(); - let didTimeout = false; - if (frameDeadline - currentTime <= 0) { - // There's no time left in this idle period. Check if the callback has - // a timeout and whether it's been exceeded. - if (timeoutTime !== -1 && timeoutTime <= currentTime) { - // Exceeded the timeout. Invoke the callback even though there's no - // time left. - didTimeout = true; - } else { - // No timeout. - if (!isAnimationFrameScheduled) { - // Schedule another animation callback so we retry later. - isAnimationFrameScheduled = true; - requestAnimationFrame(animationTick); + let keepRunningCallbacks = true; + + while (keepRunningCallbacks) { + // call any timed out callbacks, until none left have timed out. + callTimedOutCallbacks(); + + // check if we have any idle time, and if so call some callbacks + const currentTime = now(); + const idleTimeLeft = frameDeadline - currentTime > 0; + if (idleTimeLeft) { + // call the serial callback first if there is one + let nextCallback = scheduledSerialCallback; + timeoutTime = -1; + scheduledSerialCallback = null; + if (nextCallback === null) { + // if no serial callback was scheduled, run a deferred callback + nextCallback = pendingDeferredCallbacks.pop(); } - // Exit without invoking the callback. - return; - } - } else { - // There's still time left in this idle period. - didTimeout = false; - } + if (nextCallback) { + frameDeadlineObject.didTimeout = false; + isCurrentlyRunningCallback = true; + nextCallback(frameDeadlineObject); + isCurrentlyRunningCallback = false; + } else { + // There are no more scheduled callbacks. + // Our work here is done. + keepRunningCallbacks = false; + } + } else { + // No idle time left in this frame. + // Schedule another animation callback so we retry later. + isAnimationFrameScheduled = true; + requestAnimationFrame(animationTick); - const callback = scheduledCallback; - timeoutTime = -1; - scheduledCallback = null; - if (callback !== null) { - frameDeadlineObject.didTimeout = didTimeout; - isCurrentlyRunningCallback = true; - callback(frameDeadlineObject); - isCurrentlyRunningCallback = false; + keepRunningCallbacks = false; + } } }; + // Assumes that we have addEventListener in this environment. Might need // something better for old IE. window.addEventListener('message', idleTick, false); @@ -196,6 +259,23 @@ if (!ExecutionEnvironment.canUseDOM) { } }; + /** + * This method is similar to requestIdleCallback. 'Deferred' callbacks will + * be called after the 'serial' priority callbacks have been cleared, with + * additional priority given to callbacks which are past their timeout. + */ + scheduleDeferredCallback = function( + callback: (deadline: Deadline) => void, + options?: {timeout: number}, + ): number { + const deferredCallbackTimeoutTime = + options && typeof options.timeout === 'number' ? options.timeout : -1; + pendingDeferredCallbacks.push({ + deferredCallback: callback, + deferredCallbackTimeoutTime, + }); + }; + /** * 'Serial' callbacks are distinct from regular callbacks because they rely on * all previous 'serial' callbacks having been evaluated. @@ -207,21 +287,15 @@ if (!ExecutionEnvironment.canUseDOM) { callback: (deadline: Deadline) => void, options?: {timeout: number}, ): number { - // Handling multiple callbacks: - // For now we implement the behavior expected when the callbacks are - // serial updates, such that each update relies on the previous ones - // having been called before it runs. - // So we call anything in the queue before the latest callback - let previousCallback; let timeoutTimeFromPreviousCallback; - if (scheduledCallback !== null) { + if (scheduledSerialCallback !== null) { // If we have previous callback, save it and handle it below timeoutTimeFromPreviousCallback = timeoutTime; - previousCallback = scheduledCallback; + previousCallback = scheduledSerialCallback; } // Then set up the next callback, and update timeoutTime - scheduledCallback = callback; + scheduledSerialCallback = callback; if (options != null && typeof options.timeout === 'number') { timeoutTime = now() + options.timeout; } else { @@ -278,9 +352,20 @@ if (!ExecutionEnvironment.canUseDOM) { cancelSerialCallback = function() { isIdleScheduled = false; - scheduledCallback = null; + scheduledSerialCallback = null; timeoutTime = -1; }; + + cancelDeferredCallback = function(callback) { + const index = pendingDeferredCallbacks.indexOf(callback); + pendingDeferredCallbacks.splice(index, 1); + }; } -export {now, scheduleSerialCallback, cancelSerialCallback}; +export { + now, + scheduleSerialCallback, + cancelSerialCallback, + scheduleDeferredCallback, + cancelDeferredCallback, +};