From d376543b2ab01854c51fb9009b8f59bcb32ad5e8 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Tue, 24 Apr 2018 10:59:56 -0700 Subject: [PATCH 01/14] 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 b15e5f4428ee5..2555d775e01f9 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 1ed60b0fb318e..f1cfc992083df 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 fea65d60b5236e6660b1f6ebacfbb7d613506e5d Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 25 Apr 2018 08:53:37 -0700 Subject: [PATCH 02/14] 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 2555d775e01f9..3976f32fe1f99 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 5f450b8437b11d08fab359a6d1db66040cc46b79 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 25 Apr 2018 08:56:34 -0700 Subject: [PATCH 03/14] 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 3976f32fe1f99..575bdbc8c7b91 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 079bb73eb680aaf82ff5132d84ef426d2599597a Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 25 Apr 2018 09:02:13 -0700 Subject: [PATCH 04/14] 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 f1cfc992083df..f6429ed94939e 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 df6624b0a1cf8c982404302afb86ae30c7c88527 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 25 Apr 2018 14:25:48 -0700 Subject: [PATCH 05/14] 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. --- .../react-scheduler/src/ReactScheduler.js | 85 ++++++-- .../src/__tests__/ReactScheduler-test.js | 182 +++++++++++++++--- 2 files changed, 223 insertions(+), 44 deletions(-) diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index 575bdbc8c7b91..9a187bcdd62f3 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -20,7 +20,7 @@ * - 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 */ @@ -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 a queue of pending callbacks + let pendingCallbacks = []; 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 @@ -200,35 +200,78 @@ if (!ExecutionEnvironment.canUseDOM) { 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 rIC + // add this callback to a pending queue and run after we exit + pendingCallbacks.push({ + pendingCallback: previousCallback, + pendingCallbackTimeout: prevCallbackTimeout, + }); + } else { + frameDeadlineObject.didTimeout = + timeoutTimeFromPreviousCallback !== -1 && + timeoutTimeFromPreviousCallback <= now(); + isCurrentlyRunningCallback = true; + previousCallback(frameDeadlineObject); + isCurrentlyRunningCallback = false; + while (pendingCallbacks.length) { + // the callback recursively called rIC and new callbacks are pending + const { + pendingCallback, + pendingCallbackTimeout, + } = pendingCallbacks.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. isAnimationFrameScheduled = true; - requestAnimationFrame(animationTick); + return requestAnimationFrame(animationTick); } return 0; }; cIC = function() { isIdleScheduled = false; - resetScheduledCallback(); + scheduledCallback = null; + timeoutTime = -1; }; } diff --git a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js index f6429ed94939e..f9c557e8f6406 100644 --- a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js +++ b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js @@ -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 {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'); + }); + + it('schedules callbacks in correct order when a callback uses rIC before its own logic', () => { + const {rIC} = ReactScheduler; + const callbackLog = []; + const callbackA = jest.fn(() => { + callbackLog.push('A'); + rIC(callbackC); + }); + const callbackB = jest.fn(() => { + callbackLog.push('B'); + }); + const callbackC = jest.fn(() => { + callbackLog.push('C'); + }); + + rIC(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 + rIC(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 rIC calls', () => { + const {rIC} = ReactScheduler; + const callbackLog = []; + const callbackA = jest.fn(() => { + callbackLog.push('A'); + rIC(callbackC); + rIC(callbackD); + }); + const callbackB = jest.fn(() => { + callbackLog.push('B'); + rIC(callbackE); + rIC(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'); + }); + + rIC(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 + rIC(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 {rIC} = ReactScheduler; + const callbackLog = []; + const callbackA = jest.fn(() => { + // rIC should wait to flush any more until this callback finishes + rIC(callbackC); + callbackLog.push('A'); + }); + const callbackB = jest.fn(() => callbackLog.push('B')); + const callbackC = jest.fn(() => callbackLog.push('C')); + + rIC(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 + rIC(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 rIC to schedule themselves', () => { + const {rIC} = ReactScheduler; + const callbackLog = []; + let callbackAIterations = 0; + const callbackA = jest.fn(() => { + if (callbackAIterations < 1) { + rIC(callbackA); + } + callbackLog.push('A' + callbackAIterations); + callbackAIterations++; + }); + 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 + // callbackA scheduled callbackA again, which flushes callbackB + rIC(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 c1278be0c1256d4711b8807e5bcf314f7165599d Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Mon, 30 Apr 2018 11:35:44 -0700 Subject: [PATCH 06/14] comment wording nit --- packages/react-scheduler/src/ReactScheduler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index 9a187bcdd62f3..9a1d6457a4879 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -94,7 +94,7 @@ if (!ExecutionEnvironment.canUseDOM) { let isIdleScheduled = false; let timeoutTime = -1; let isCurrentlyRunningCallback = false; - // We may need to keep a queue of pending callbacks + // We keep a queue of pending callbacks let pendingCallbacks = []; let isAnimationFrameScheduled = false; From 8283c95a76966b561b0cf9bbf2c3f0bf90519e39 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Tue, 1 May 2018 10:28:25 -0700 Subject: [PATCH 07/14] =?UTF-8?q?=C2=A0remove=20destructuring?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/react-scheduler/src/ReactScheduler.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index 9a1d6457a4879..9e7ca1dbc7774 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -242,10 +242,9 @@ if (!ExecutionEnvironment.canUseDOM) { isCurrentlyRunningCallback = false; while (pendingCallbacks.length) { // the callback recursively called rIC and new callbacks are pending - const { - pendingCallback, - pendingCallbackTimeout, - } = pendingCallbacks.shift(); + const callbackConfig = pendingCallbacks.shift(); + const pendingCallback = callbackConfig.pendingCallback; + const pendingCallbackTimeout = callbackConfig.pendingCallbackTimeout; // TODO: pull this into helper method frameDeadlineObject.didTimeout = pendingCallbackTimeout !== -1 && pendingCallbackTimeout <= now(); From eb0110fdfd93975af0d24dee62e03192ac14f7b8 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Tue, 1 May 2018 11:15:56 -0700 Subject: [PATCH 08/14] Handle errors thrown by callbacks in `schedule` **what is the change?:** We may call a sequence of callbacks in our `schedule` helper, and this catches any errors and re-throws them later, so we don't get interrupted or left in an invalid state. **why make this change?:** If you have callbacks A, B, and C scheduled, and B throws, we still want C to get called. I would prefer if we could find a way to throw and continue syncronously but for now this approach, of deferring the error, will work. **test plan:** Added a unit test. --- .../react-scheduler/src/ReactScheduler.js | 24 ++++++++----- .../src/__tests__/ReactScheduler-test.js | 35 +++++++++++++++++++ 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index 9e7ca1dbc7774..fa42997b1e103 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -114,6 +114,18 @@ if (!ExecutionEnvironment.canUseDOM) { }, }; + const safelyCallScheduledCallback = function(callback) { + isCurrentlyRunningCallback = true; + try { + callback(frameDeadlineObject); + isCurrentlyRunningCallback = false; + } catch (e) { + isCurrentlyRunningCallback = false; + // Still throw it, but not in this frame. + setTimeout(() => {throw e;}); + } + }; + // We use the postMessage trick to defer idle work until after the repaint. const messageKey = '__reactIdleCallback$' + @@ -156,9 +168,7 @@ if (!ExecutionEnvironment.canUseDOM) { scheduledCallback = null; if (callback !== null) { frameDeadlineObject.didTimeout = didTimeout; - isCurrentlyRunningCallback = true; - callback(frameDeadlineObject); - isCurrentlyRunningCallback = false; + safelyCallScheduledCallback(callback); } }; // Assumes that we have addEventListener in this environment. Might need @@ -237,9 +247,7 @@ if (!ExecutionEnvironment.canUseDOM) { frameDeadlineObject.didTimeout = timeoutTimeFromPreviousCallback !== -1 && timeoutTimeFromPreviousCallback <= now(); - isCurrentlyRunningCallback = true; - previousCallback(frameDeadlineObject); - isCurrentlyRunningCallback = false; + safelyCallScheduledCallback(previousCallback); while (pendingCallbacks.length) { // the callback recursively called rIC and new callbacks are pending const callbackConfig = pendingCallbacks.shift(); @@ -248,9 +256,7 @@ if (!ExecutionEnvironment.canUseDOM) { // TODO: pull this into helper method frameDeadlineObject.didTimeout = pendingCallbackTimeout !== -1 && pendingCallbackTimeout <= now(); - isCurrentlyRunningCallback = true; - pendingCallback(frameDeadlineObject); - isCurrentlyRunningCallback = false; + safelyCallScheduledCallback(pendingCallback); } } } diff --git a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js index f9c557e8f6406..8ff46cc8b5735 100644 --- a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js +++ b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js @@ -210,6 +210,41 @@ describe('ReactScheduler', () => { expect(callbackLog[1]).toBe('B'); expect(callbackLog[2]).toBe('A1'); }); + + describe('handling errors', () => { + it('flushes scheduled callbacks even if one throws error', () => { + const {rIC} = ReactScheduler; + const callbackLog = []; + const callbackA = jest.fn(() => { + callbackLog.push('A'); + rIC(callbackC); + throw new Error('dummy error A'); + }); + const callbackB = jest.fn(() => { + callbackLog.push('B'); + }); + const callbackC = jest.fn(() => { + callbackLog.push('C'); + }); + + rIC(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 + // even when callbackA throws an error, we successfully call callbackB + rIC(callbackB); + expect(callbackLog.length).toBe(2); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + // after a delay, throws the error and calls the latest callback passed + expect(() => jest.runAllTimers()).toThrowError('dummy error A'); + expect(callbackLog.length).toBe(3); + expect(callbackLog[0]).toBe('A'); + expect(callbackLog[1]).toBe('B'); + expect(callbackLog[2]).toBe('C'); + }); + }); }); // TODO: test cIC and now From 0c47fe2bcb9b8c6cc5d2e34de848f3037a183dad Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Tue, 1 May 2018 14:45:04 -0700 Subject: [PATCH 09/14] Add initial test for cIC **what is the change?:** see title **why make this change?:** We are about to add functionality to cIC, where we take an id. This is in preparation for when we support 'deferred' as well as 'serial' callbacks. **test plan:** Run the test. --- .../src/__tests__/ReactScheduler-test.js | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js index 8ff46cc8b5735..6221549bdd3c0 100644 --- a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js +++ b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js @@ -247,5 +247,35 @@ describe('ReactScheduler', () => { }); }); - // TODO: test cIC and now + describe('cIC', () => { + // TODO: return an id from rIC and use in cIC + // and test this. + it('cancels the scheduled callback', () => { + const {rIC, cIC} = ReactScheduler; + const cb = jest.fn(); + rIC(cb); + expect(cb.mock.calls.length).toBe(0); + cIC(); + jest.runAllTimers(); + expect(cb.mock.calls.length).toBe(0); + }); + + it('when one callback cancels the next one', () => { + const {rIC, cIC} = ReactScheduler; + const cbA = jest.fn(() => { + cIC(); + }); + const cbB = jest.fn(); + rIC(cbA); + expect(cbA.mock.calls.length).toBe(0); + rIC(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 'now' }); From e96d0e338fde16b93ed0f664eb5b3bfb2d162550 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Tue, 1 May 2018 16:18:43 -0700 Subject: [PATCH 10/14] Add stronger types and refactoring in `ReactScheduler` **what is the change?:** Some various clean-up changes to pave the way for adding an 'id' for each callback which can be used to cancel the callback. **why make this change?:** To make the next diff easier. **test plan:** Ran the tests. --- .../react-scheduler/src/ReactScheduler.js | 91 ++++++++++++------- 1 file changed, 58 insertions(+), 33 deletions(-) diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index fa42997b1e103..16d459c1fe9a1 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -34,6 +34,11 @@ // The frame rate is dynamically adjusted. import type {Deadline} from 'react-reconciler'; +type CallbackConfigType = {| + scheduledCallback: (Deadline) => void, + timeoutTime: number, + // id: string, // used for cancelling +|}; import ExecutionEnvironment from 'fbjs/lib/ExecutionEnvironment'; import warning from 'fbjs/lib/warning'; @@ -90,12 +95,19 @@ if (!ExecutionEnvironment.canUseDOM) { } else { // Always polyfill requestIdleCallback and cancelIdleCallback - let scheduledCallback = null; + let scheduledCallbackConfig: CallbackConfigType | null = null; + const getCallbackId = function(): string { + const callbackId = + '__scheduledCallbackId$' + + Math.random() + .toString(36) + .slice(2); + return callbackId; + }; let isIdleScheduled = false; - let timeoutTime = -1; let isCurrentlyRunningCallback = false; // We keep a queue of pending callbacks - let pendingCallbacks = []; + let pendingCallbacks: Array = []; let isAnimationFrameScheduled = false; @@ -106,7 +118,15 @@ if (!ExecutionEnvironment.canUseDOM) { let previousFrameTime = 33; let activeFrameTime = 33; - const frameDeadlineObject = { + // When a callback is scheduled, we register it by adding it's id to this + // object. + // If the user calls 'cIC' with the id of that callback, it will be + // unregistered by removing the id from this object. + // Then we skip calling any callback which is not registered. + // This means cancelling is an O(1) time complexity instead of O(n). + const registeredCallbackIds: { [number]: boolean } = {}; + + const frameDeadlineObject: Deadline = { didTimeout: false, timeRemaining() { const remaining = frameDeadline - now(); @@ -122,7 +142,9 @@ if (!ExecutionEnvironment.canUseDOM) { } catch (e) { isCurrentlyRunningCallback = false; // Still throw it, but not in this frame. - setTimeout(() => {throw e;}); + setTimeout(() => { + throw e; + }); } }; @@ -137,9 +159,14 @@ if (!ExecutionEnvironment.canUseDOM) { return; } + if (scheduledCallbackConfig === null) { + return; + } + isIdleScheduled = false; const currentTime = now(); + const timeoutTime = scheduledCallbackConfig.timeoutTime; let didTimeout = false; if (frameDeadline - currentTime <= 0) { // There's no time left in this idle period. Check if the callback has @@ -163,9 +190,8 @@ if (!ExecutionEnvironment.canUseDOM) { didTimeout = false; } - const callback = scheduledCallback; - timeoutTime = -1; - scheduledCallback = null; + const callback = scheduledCallbackConfig.scheduledCallback; + scheduledCallbackConfig = null; if (callback !== null) { frameDeadlineObject.didTimeout = didTimeout; safelyCallScheduledCallback(callback); @@ -216,44 +242,44 @@ if (!ExecutionEnvironment.canUseDOM) { // 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; + let previouslyScheduledCallbackConfig; + if (scheduledCallbackConfig !== null) { + // If we have previous callback config, save it and handle it below + previouslyScheduledCallbackConfig = scheduledCallbackConfig; } - // Then set up the next callback, and update timeoutTime - scheduledCallback = callback; + // Then set up the next callback config + let timeoutTime = -1; if (options != null && typeof options.timeout === 'number') { timeoutTime = now() + options.timeout; - } else { - timeoutTime = -1; } + scheduledCallbackConfig = { + scheduledCallback: callback, + timeoutTime, + }; // If we have previousCallback, call it. This may trigger recursion. if ( - previousCallback && - typeof timeoutTimeFromPreviousCallback === 'number' + previouslyScheduledCallbackConfig && + // make flow happy + typeof previouslyScheduledCallbackConfig.timeoutTime === 'number' ) { - const prevCallbackTimeout: number = timeoutTimeFromPreviousCallback; + const previousCallbackTimeout: number = + previouslyScheduledCallbackConfig.timeoutTime; + const previousCallback = + previouslyScheduledCallbackConfig.scheduledCallback; if (isCurrentlyRunningCallback) { // we are inside a recursive call to rIC // add this callback to a pending queue and run after we exit - pendingCallbacks.push({ - pendingCallback: previousCallback, - pendingCallbackTimeout: prevCallbackTimeout, - }); + pendingCallbacks.push(previouslyScheduledCallbackConfig); } else { frameDeadlineObject.didTimeout = - timeoutTimeFromPreviousCallback !== -1 && - timeoutTimeFromPreviousCallback <= now(); + previousCallbackTimeout !== -1 && + previousCallbackTimeout <= now(); safelyCallScheduledCallback(previousCallback); while (pendingCallbacks.length) { // the callback recursively called rIC and new callbacks are pending const callbackConfig = pendingCallbacks.shift(); - const pendingCallback = callbackConfig.pendingCallback; - const pendingCallbackTimeout = callbackConfig.pendingCallbackTimeout; - // TODO: pull this into helper method + const pendingCallback = callbackConfig.scheduledCallback; + const pendingCallbackTimeout = callbackConfig.timeoutTime; frameDeadlineObject.didTimeout = pendingCallbackTimeout !== -1 && pendingCallbackTimeout <= now(); safelyCallScheduledCallback(pendingCallback); @@ -268,15 +294,14 @@ if (!ExecutionEnvironment.canUseDOM) { // might want to still have setTimeout trigger rIC as a backup to ensure // that we keep performing work. isAnimationFrameScheduled = true; - return requestAnimationFrame(animationTick); + requestAnimationFrame(animationTick); } return 0; }; cIC = function() { isIdleScheduled = false; - scheduledCallback = null; - timeoutTime = -1; + scheduledCallbackConfig = null; }; } From 26b82c80cc7b76ef688388cb2f080bc38ea4d19f Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Tue, 1 May 2018 16:36:19 -0700 Subject: [PATCH 11/14] Return an id when scheduling callback; use it for cancelling callback **what is the change?:** See title. **why make this change?:** When we support multiple callbacks you will need to use the callback id to cancel a specific callback. **test plan:** Tests were updated, ran all tests. --- .../react-scheduler/src/ReactScheduler.js | 56 +++++++++++-------- .../src/__tests__/ReactScheduler-test.js | 12 ++-- 2 files changed, 41 insertions(+), 27 deletions(-) diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index 16d459c1fe9a1..214ce3d414002 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -35,9 +35,9 @@ import type {Deadline} from 'react-reconciler'; type CallbackConfigType = {| - scheduledCallback: (Deadline) => void, + scheduledCallback: Deadline => void, timeoutTime: number, - // id: string, // used for cancelling + callbackId: number, // used for cancelling |}; import ExecutionEnvironment from 'fbjs/lib/ExecutionEnvironment'; @@ -95,14 +95,14 @@ if (!ExecutionEnvironment.canUseDOM) { } else { // Always polyfill requestIdleCallback and cancelIdleCallback + // Number.MAX_SAFE_INTEGER is not supported in IE + const MAX_SAFE_INTEGER = Number.MAX_SAFE_INTEGER || 9007199254740991; + let callbackIdCounter = 1; let scheduledCallbackConfig: CallbackConfigType | null = null; - const getCallbackId = function(): string { - const callbackId = - '__scheduledCallbackId$' + - Math.random() - .toString(36) - .slice(2); - return callbackId; + const getCallbackId = function(): number { + callbackIdCounter = + callbackIdCounter >= MAX_SAFE_INTEGER ? 1 : callbackIdCounter + 1; + return callbackIdCounter; }; let isIdleScheduled = false; let isCurrentlyRunningCallback = false; @@ -124,7 +124,7 @@ if (!ExecutionEnvironment.canUseDOM) { // unregistered by removing the id from this object. // Then we skip calling any callback which is not registered. // This means cancelling is an O(1) time complexity instead of O(n). - const registeredCallbackIds: { [number]: boolean } = {}; + const registeredCallbackIds: {[number]: boolean} = {}; const frameDeadlineObject: Deadline = { didTimeout: false, @@ -134,12 +134,18 @@ if (!ExecutionEnvironment.canUseDOM) { }, }; - const safelyCallScheduledCallback = function(callback) { + const safelyCallScheduledCallback = function(callback, callbackId) { + if (!registeredCallbackIds[callbackId]) { + // ignore cancelled callbacks + return; + } isCurrentlyRunningCallback = true; try { callback(frameDeadlineObject); + delete registeredCallbackIds[callbackId]; isCurrentlyRunningCallback = false; } catch (e) { + delete registeredCallbackIds[callbackId]; isCurrentlyRunningCallback = false; // Still throw it, but not in this frame. setTimeout(() => { @@ -190,11 +196,12 @@ if (!ExecutionEnvironment.canUseDOM) { didTimeout = false; } - const callback = scheduledCallbackConfig.scheduledCallback; + const scheduledCallback = scheduledCallbackConfig.scheduledCallback; + const scheduledCallbackId = scheduledCallbackConfig.callbackId; scheduledCallbackConfig = null; - if (callback !== null) { + if (scheduledCallback !== null && typeof scheduledCallbackId === 'number') { frameDeadlineObject.didTimeout = didTimeout; - safelyCallScheduledCallback(callback); + safelyCallScheduledCallback(scheduledCallback, scheduledCallbackId); } }; // Assumes that we have addEventListener in this environment. Might need @@ -252,10 +259,13 @@ if (!ExecutionEnvironment.canUseDOM) { if (options != null && typeof options.timeout === 'number') { timeoutTime = now() + options.timeout; } + const latestCallbackId = getCallbackId(); scheduledCallbackConfig = { scheduledCallback: callback, timeoutTime, + callbackId: latestCallbackId, }; + registeredCallbackIds[latestCallbackId] = true; // If we have previousCallback, call it. This may trigger recursion. if ( previouslyScheduledCallbackConfig && @@ -271,10 +281,10 @@ if (!ExecutionEnvironment.canUseDOM) { // add this callback to a pending queue and run after we exit pendingCallbacks.push(previouslyScheduledCallbackConfig); } else { + const prevCallbackId = previouslyScheduledCallbackConfig.callbackId; frameDeadlineObject.didTimeout = - previousCallbackTimeout !== -1 && - previousCallbackTimeout <= now(); - safelyCallScheduledCallback(previousCallback); + previousCallbackTimeout !== -1 && previousCallbackTimeout <= now(); + safelyCallScheduledCallback(previousCallback, prevCallbackId); while (pendingCallbacks.length) { // the callback recursively called rIC and new callbacks are pending const callbackConfig = pendingCallbacks.shift(); @@ -282,7 +292,10 @@ if (!ExecutionEnvironment.canUseDOM) { const pendingCallbackTimeout = callbackConfig.timeoutTime; frameDeadlineObject.didTimeout = pendingCallbackTimeout !== -1 && pendingCallbackTimeout <= now(); - safelyCallScheduledCallback(pendingCallback); + safelyCallScheduledCallback( + pendingCallback, + callbackConfig.callbackId, + ); } } } @@ -296,12 +309,11 @@ if (!ExecutionEnvironment.canUseDOM) { isAnimationFrameScheduled = true; requestAnimationFrame(animationTick); } - return 0; + return latestCallbackId; }; - cIC = function() { - isIdleScheduled = false; - scheduledCallbackConfig = null; + cIC = function(callbackId: number) { + delete registeredCallbackIds[callbackId]; }; } diff --git a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js index 6221549bdd3c0..8a7d126e9ee03 100644 --- a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js +++ b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js @@ -248,33 +248,35 @@ describe('ReactScheduler', () => { }); describe('cIC', () => { - // TODO: return an id from rIC and use in cIC - // and test this. it('cancels the scheduled callback', () => { const {rIC, cIC} = ReactScheduler; const cb = jest.fn(); - rIC(cb); + const callbackId = rIC(cb); expect(cb.mock.calls.length).toBe(0); - cIC(); + cIC(callbackId); jest.runAllTimers(); expect(cb.mock.calls.length).toBe(0); }); + // TODO: this test will be easier to implement once we support deferred + /** it('when one callback cancels the next one', () => { const {rIC, cIC} = ReactScheduler; const cbA = jest.fn(() => { + // How to get the callback id? cIC(); }); const cbB = jest.fn(); rIC(cbA); expect(cbA.mock.calls.length).toBe(0); - rIC(cbB); + callbackBId = rIC(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 'now' From 64b57b1b35747725e4bb3fd4a3f3a57cb74045de Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 2 May 2018 11:27:42 -0700 Subject: [PATCH 12/14] Use 'toEqual' instead of 'toBe' in schedule tests; more precise/concise --- .../src/__tests__/ReactScheduler-test.js | 55 +++++-------------- 1 file changed, 13 insertions(+), 42 deletions(-) diff --git a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js index 8a7d126e9ee03..2b304e0537f8b 100644 --- a/packages/react-scheduler/src/__tests__/ReactScheduler-test.js +++ b/packages/react-scheduler/src/__tests__/ReactScheduler-test.js @@ -60,16 +60,13 @@ describe('ReactScheduler', () => { const callbackB = jest.fn(() => callbackLog.push('B')); rIC(callbackA); // initially waits to call the callback - expect(callbackLog.length).toBe(0); + expect(callbackLog).toEqual([]); // when second callback is passed, flushes first one rIC(callbackB); - expect(callbackLog.length).toBe(1); - expect(callbackLog[0]).toBe('A'); + expect(callbackLog).toEqual(['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'); + expect(callbackLog).toEqual(['A', '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'); @@ -98,15 +95,10 @@ describe('ReactScheduler', () => { // when second callback is passed, flushes first one // callbackA scheduled callbackC, which flushes callbackB rIC(callbackB); - expect(callbackLog.length).toBe(2); - expect(callbackLog[0]).toBe('A'); - expect(callbackLog[1]).toBe('B'); + expect(callbackLog).toEqual(['A', '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'); + expect(callbackLog).toEqual(['A', 'B', 'C']); }); it('schedules callbacks in correct order when callbacks have many nested rIC calls', () => { @@ -141,16 +133,10 @@ describe('ReactScheduler', () => { // when second callback is passed, flushes first one // callbackA scheduled callbackC, which flushes callbackB rIC(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'); + expect(callbackLog).toEqual(['A', 'B', 'C', 'D', 'E']); // after a delay, calls the latest callback passed jest.runAllTimers(); - expect(callbackLog.length).toBe(6); - expect(callbackLog[5]).toBe('F'); + expect(callbackLog).toEqual(['A', 'B', 'C', 'D', 'E', 'F']); }); it('allows each callback finish running before flushing others', () => { @@ -170,15 +156,10 @@ describe('ReactScheduler', () => { // when second callback is passed, flushes first one // callbackA scheduled callbackC, which flushes callbackB rIC(callbackB); - expect(callbackLog.length).toBe(2); - expect(callbackLog[0]).toBe('A'); - expect(callbackLog[1]).toBe('B'); + expect(callbackLog).toEqual(['A', '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'); + expect(callbackLog).toEqual(['A', 'B', 'C']); }); it('schedules callbacks in correct order when they use rIC to schedule themselves', () => { @@ -200,15 +181,10 @@ describe('ReactScheduler', () => { // when second callback is passed, flushes first one // callbackA scheduled callbackA again, which flushes callbackB rIC(callbackB); - expect(callbackLog.length).toBe(2); - expect(callbackLog[0]).toBe('A0'); - expect(callbackLog[1]).toBe('B'); + expect(callbackLog).toEqual(['A0', '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'); + expect(callbackLog).toEqual(['A0', 'B', 'A1']); }); describe('handling errors', () => { @@ -234,15 +210,10 @@ describe('ReactScheduler', () => { // callbackA scheduled callbackC, which flushes callbackB // even when callbackA throws an error, we successfully call callbackB rIC(callbackB); - expect(callbackLog.length).toBe(2); - expect(callbackLog[0]).toBe('A'); - expect(callbackLog[1]).toBe('B'); + expect(callbackLog).toEqual(['A', 'B']); // after a delay, throws the error and calls the latest callback passed expect(() => jest.runAllTimers()).toThrowError('dummy error A'); - expect(callbackLog.length).toBe(3); - expect(callbackLog[0]).toBe('A'); - expect(callbackLog[1]).toBe('B'); - expect(callbackLog[2]).toBe('C'); + expect(callbackLog).toEqual(['A', 'B', 'C']); }); }); }); From 8703f2154dec5c5836e0794e9edce9019d4000a9 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 2 May 2018 11:32:45 -0700 Subject: [PATCH 13/14] Use Map instead of object for registeredCallbackIds --- packages/react-scheduler/src/ReactScheduler.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index 214ce3d414002..552d234859283 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -124,7 +124,7 @@ if (!ExecutionEnvironment.canUseDOM) { // unregistered by removing the id from this object. // Then we skip calling any callback which is not registered. // This means cancelling is an O(1) time complexity instead of O(n). - const registeredCallbackIds: {[number]: boolean} = {}; + const registeredCallbackIds: Map = new Map(); const frameDeadlineObject: Deadline = { didTimeout: false, @@ -135,17 +135,17 @@ if (!ExecutionEnvironment.canUseDOM) { }; const safelyCallScheduledCallback = function(callback, callbackId) { - if (!registeredCallbackIds[callbackId]) { + if (!registeredCallbackIds.get(callbackId)) { // ignore cancelled callbacks return; } isCurrentlyRunningCallback = true; try { callback(frameDeadlineObject); - delete registeredCallbackIds[callbackId]; + registeredCallbackIds.delete(callbackId); isCurrentlyRunningCallback = false; } catch (e) { - delete registeredCallbackIds[callbackId]; + registeredCallbackIds.delete(callbackId); isCurrentlyRunningCallback = false; // Still throw it, but not in this frame. setTimeout(() => { @@ -265,7 +265,7 @@ if (!ExecutionEnvironment.canUseDOM) { timeoutTime, callbackId: latestCallbackId, }; - registeredCallbackIds[latestCallbackId] = true; + registeredCallbackIds.set(latestCallbackId, true); // If we have previousCallback, call it. This may trigger recursion. if ( previouslyScheduledCallbackConfig && @@ -313,7 +313,7 @@ if (!ExecutionEnvironment.canUseDOM) { }; cIC = function(callbackId: number) { - delete registeredCallbackIds[callbackId]; + registeredCallbackIds.delete(callbackId); }; } From 5ba093b8ffa2c7a27025d00a85a2b9c8c735b298 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 2 May 2018 12:25:36 -0700 Subject: [PATCH 14/14] Restructure 'schedule' rIC code for clarity **what is the change?:** We previously had the following logic: - pull any old callback out of the 'scheduledCallback' variable - put the new callback into 'scheduledCallback' - call the old callback if we had found one. Splitting out the logic for handling the old callback into two blocks was hard to read, so we restructured as so: - if no old callback was found, just put the new callback into the 'scheduledCallback' variable. - Otherwise, if we do find an old callback, then: - pull any old callback out of the 'scheduledCallback' variable - put the new callback into 'scheduledCallback' - call the old callback **why make this change?:** Code clarity **test plan:** Ran the tests --- .../react-scheduler/src/ReactScheduler.js | 52 +++++++++++-------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/packages/react-scheduler/src/ReactScheduler.js b/packages/react-scheduler/src/ReactScheduler.js index 552d234859283..0486d466f0233 100644 --- a/packages/react-scheduler/src/ReactScheduler.js +++ b/packages/react-scheduler/src/ReactScheduler.js @@ -249,29 +249,37 @@ if (!ExecutionEnvironment.canUseDOM) { // having been called before it runs. // So we call anything in the queue before the latest callback - let previouslyScheduledCallbackConfig; - if (scheduledCallbackConfig !== null) { - // If we have previous callback config, save it and handle it below - previouslyScheduledCallbackConfig = scheduledCallbackConfig; - } - // Then set up the next callback config - let timeoutTime = -1; - if (options != null && typeof options.timeout === 'number') { - timeoutTime = now() + options.timeout; - } const latestCallbackId = getCallbackId(); - scheduledCallbackConfig = { - scheduledCallback: callback, - timeoutTime, - callbackId: latestCallbackId, - }; - registeredCallbackIds.set(latestCallbackId, true); - // If we have previousCallback, call it. This may trigger recursion. - if ( - previouslyScheduledCallbackConfig && - // make flow happy - typeof previouslyScheduledCallbackConfig.timeoutTime === 'number' - ) { + if (scheduledCallbackConfig === null) { + // Set up the next callback config + let timeoutTime = -1; + if (options != null && typeof options.timeout === 'number') { + timeoutTime = now() + options.timeout; + } + scheduledCallbackConfig = { + scheduledCallback: callback, + timeoutTime, + callbackId: latestCallbackId, + }; + registeredCallbackIds.set(latestCallbackId, true); + } else { + // If we have a previous callback config, we call that and then schedule + // the latest callback. + const previouslyScheduledCallbackConfig = scheduledCallbackConfig; + + // Then set up the next callback config + let timeoutTime = -1; + if (options != null && typeof options.timeout === 'number') { + timeoutTime = now() + options.timeout; + } + scheduledCallbackConfig = { + scheduledCallback: callback, + timeoutTime, + callbackId: latestCallbackId, + }; + registeredCallbackIds.set(latestCallbackId, true); + + // If we have previousCallback, call it. This may trigger recursion. const previousCallbackTimeout: number = previouslyScheduledCallbackConfig.timeoutTime; const previousCallback =