From 1137a0ac20f66f6b4d0e80adb34fd04abf20bf88 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 27 Feb 2019 15:13:27 -0800 Subject: [PATCH 1/8] Use set and clear debouncer upon completion. Fixes #5250 --- lib/utils/debounce.html | 32 ++++++++++++++++++++++++++++ lib/utils/flush.html | 30 ++------------------------ test/unit/debounce.html | 47 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 28 deletions(-) diff --git a/lib/utils/debounce.html b/lib/utils/debounce.html index 1507faa20a..48efe629a5 100644 --- a/lib/utils/debounce.html +++ b/lib/utils/debounce.html @@ -40,6 +40,7 @@ this._timer = this._asyncModule.run(() => { this._timer = null; this._callback(); + debouncerQueue.delete(this); }); } /** @@ -115,5 +116,36 @@ /** @const */ Polymer.Debouncer = Debouncer; + + let debouncerQueue = new Set(); + + /** + * Adds a `Debouncer` to a list of globally flushable tasks. + * + * @param {!Debouncer} debouncer Debouncer to enqueue + * @return {void} + */ + Polymer.enqueueDebouncer = function(debouncer) { + if (debouncerQueue.has(debouncer)) { + debouncerQueue.delete(debouncer); + } + debouncerQueue.add(debouncer); + }; + + Polymer.flushDebouncers = function() { + const didFlush = Boolean(debouncerQueue.size); + debouncerQueue.forEach(debouncer => { + try { + debouncer.flush(); + } catch(e) { + setTimeout(() => { + throw e; + }); + } + }); + debouncerQueue = new Set(); + return didFlush; + }; + })(); diff --git a/lib/utils/flush.html b/lib/utils/flush.html index 3feaad6166..0ab98780cd 100644 --- a/lib/utils/flush.html +++ b/lib/utils/flush.html @@ -8,37 +8,11 @@ subject to an additional IP rights grant found at http://polymer.github.io/PATENTS.txt --> + From 8fba53f5163fe9d6e0ef318c48319adcb96c9fba Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 27 Feb 2019 15:23:44 -0800 Subject: [PATCH 2/8] Update types --- lib/utils/debounce.html | 1 + types/lib/utils/debounce.d.ts | 6 ++++++ types/lib/utils/flush.d.ts | 7 +------ 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/utils/debounce.html b/lib/utils/debounce.html index 48efe629a5..ad6b98ff95 100644 --- a/lib/utils/debounce.html +++ b/lib/utils/debounce.html @@ -122,6 +122,7 @@ /** * Adds a `Debouncer` to a list of globally flushable tasks. * + * @memberof Polymer * @param {!Debouncer} debouncer Debouncer to enqueue * @return {void} */ diff --git a/types/lib/utils/debounce.d.ts b/types/lib/utils/debounce.d.ts index 3800afc4a2..ceb853ef08 100644 --- a/types/lib/utils/debounce.d.ts +++ b/types/lib/utils/debounce.d.ts @@ -80,4 +80,10 @@ declare namespace Polymer { */ isActive(): boolean; } + + + /** + * Adds a `Debouncer` to a list of globally flushable tasks. + */ + function enqueueDebouncer(debouncer: Debouncer): void; } diff --git a/types/lib/utils/flush.d.ts b/types/lib/utils/flush.d.ts index fde1037a6f..6360fcd8b6 100644 --- a/types/lib/utils/flush.d.ts +++ b/types/lib/utils/flush.d.ts @@ -12,16 +12,11 @@ // tslint:disable:variable-name Describing an API that's defined elsewhere. /// +/// declare namespace Polymer { - /** - * Adds a `Polymer.Debouncer` to a list of globally flushable tasks. - */ - function enqueueDebouncer(debouncer: Polymer.Debouncer): void; - - /** * Forces several classes of asynchronously queued tasks to flush: * - Debouncers added via `enqueueDebouncer` From f4df46634213dbc1528e438ac7fef4169979e1b3 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 27 Feb 2019 15:48:09 -0800 Subject: [PATCH 3/8] Add comments --- lib/utils/debounce.html | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/utils/debounce.html b/lib/utils/debounce.html index ad6b98ff95..54fc859623 100644 --- a/lib/utils/debounce.html +++ b/lib/utils/debounce.html @@ -127,6 +127,8 @@ * @return {void} */ Polymer.enqueueDebouncer = function(debouncer) { + // Re-enqueued debouncers are put at the end of the queue; for Set, this + // means removing and re-adding, since forEach traverses insertion order if (debouncerQueue.has(debouncer)) { debouncerQueue.delete(debouncer); } @@ -135,6 +137,8 @@ Polymer.flushDebouncers = function() { const didFlush = Boolean(debouncerQueue.size); + // If new debouncers are added while flushing, Set.forEach will ensure + // newly added ones are also flushed debouncerQueue.forEach(debouncer => { try { debouncer.flush(); From 7485a60d0a258ebef1e88b06cd55152e828b7c18 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 27 Feb 2019 15:57:01 -0800 Subject: [PATCH 4/8] Avoid Array.fill --- test/unit/debounce.html | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/test/unit/debounce.html b/test/unit/debounce.html index 1c6e1520e5..f2fbc81935 100644 --- a/test/unit/debounce.html +++ b/test/unit/debounce.html @@ -216,16 +216,22 @@ const timeoutCallback = sinon.spy(() => actualCallbacks.push(timeoutCallback)); Polymer.enqueueDebouncer(Polymer.Debouncer.debounce(null, Polymer.Async.timeOut, timeoutCallback)); // Set of short-running debouncers enqueued in the middle of first set - const nestedCallbacks = new Array(150).fill().map((_, i) => sinon.spy(() => - actualCallbacks.push(nestedCallbacks[i]))); + const nestedCallbacks = []; + for (let i=0; i<150; i++) { + nestedCallbacks.push(sinon.spy(() => + actualCallbacks.push(nestedCallbacks[i]))); + } // First set of short-running debouncers - const microtaskCallbacks = new Array(150).fill().map((_, i) => sinon.spy(() => { - actualCallbacks.push(microtaskCallbacks[i]); - if (i === 125) { - nestedCallbacks.forEach(cb => - Polymer.enqueueDebouncer(Polymer.Debouncer.debounce(null, Polymer.Async.microTask, cb))); - } - })); + const microtaskCallbacks = []; + for (let i=0; i<150; i++) { + microtaskCallbacks.push(sinon.spy(() => { + actualCallbacks.push(microtaskCallbacks[i]); + if (i === 125) { + nestedCallbacks.forEach(cb => + Polymer.enqueueDebouncer(Polymer.Debouncer.debounce(null, Polymer.Async.microTask, cb))); + } + })); + } microtaskCallbacks.forEach(cb => Polymer.enqueueDebouncer(Polymer.Debouncer.debounce(null, Polymer.Async.microTask, cb))); // Expect short before long From 52c41e6960a8b9261a23209796bef744347522a2 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 27 Feb 2019 18:15:38 -0800 Subject: [PATCH 5/8] Fix order of flushed debouncers to match 1.x --- lib/utils/debounce.html | 20 +++++++++----- test/unit/debounce.html | 58 ++++++++++++++++------------------------- 2 files changed, 35 insertions(+), 43 deletions(-) diff --git a/lib/utils/debounce.html b/lib/utils/debounce.html index 54fc859623..01142cbd95 100644 --- a/lib/utils/debounce.html +++ b/lib/utils/debounce.html @@ -50,10 +50,19 @@ */ cancel() { if (this.isActive()) { - this._asyncModule.cancel(this._timer); + this._cancelAsync(); this._timer = null; + debouncerQueue.delete(this); } } + /** + * Cancels a debouncer's async callback. + * + * @return {void} + */ + _cancelAsync() { + this._asyncModule.cancel(/** @type {number} */(this._timer)); + } /** * Flushes an active debouncer and returns a reference to itself. * @@ -105,7 +114,9 @@ */ static debounce(debouncer, asyncModule, callback) { if (debouncer instanceof Debouncer) { - debouncer.cancel(); + // Cancel the async callback, but leave in debouncerQueue if it was + // enqueued, to maintain 1.x flush order + debouncer._cancelAsync(); } else { debouncer = new Debouncer(); } @@ -127,11 +138,6 @@ * @return {void} */ Polymer.enqueueDebouncer = function(debouncer) { - // Re-enqueued debouncers are put at the end of the queue; for Set, this - // means removing and re-adding, since forEach traverses insertion order - if (debouncerQueue.has(debouncer)) { - debouncerQueue.delete(debouncer); - } debouncerQueue.add(debouncer); }; diff --git a/test/unit/debounce.html b/test/unit/debounce.html index f2fbc81935..fbb330753c 100644 --- a/test/unit/debounce.html +++ b/test/unit/debounce.html @@ -211,47 +211,33 @@ }); suite('enqueueDebouncer & flush', function() { - function testEnqueue(shouldFlush, done) { - // Longer-running debouncer - const timeoutCallback = sinon.spy(() => actualCallbacks.push(timeoutCallback)); - Polymer.enqueueDebouncer(Polymer.Debouncer.debounce(null, Polymer.Async.timeOut, timeoutCallback)); - // Set of short-running debouncers enqueued in the middle of first set - const nestedCallbacks = []; - for (let i=0; i<150; i++) { - nestedCallbacks.push(sinon.spy(() => - actualCallbacks.push(nestedCallbacks[i]))); - } - // First set of short-running debouncers - const microtaskCallbacks = []; - for (let i=0; i<150; i++) { - microtaskCallbacks.push(sinon.spy(() => { - actualCallbacks.push(microtaskCallbacks[i]); - if (i === 125) { - nestedCallbacks.forEach(cb => - Polymer.enqueueDebouncer(Polymer.Debouncer.debounce(null, Polymer.Async.microTask, cb))); - } - })); - } - microtaskCallbacks.forEach(cb => - Polymer.enqueueDebouncer(Polymer.Debouncer.debounce(null, Polymer.Async.microTask, cb))); - // Expect short before long - let expectedCallbacks; - const actualCallbacks = []; - const verify = () => { - actualCallbacks.forEach(cb => assert.isTrue(cb.calledOnce)); - assert.deepEqual(expectedCallbacks, actualCallbacks); - done(); + + const testEnqueue = (shouldFlush, done) => { + const actualOrder = []; + let i=1; + const enqueue = (type, {db, cb} = {}) => { + cb = cb || (() => actualOrder.push(cb)); + db = Polymer.Debouncer.debounce(db, type, cb); + Polymer.enqueueDebouncer(db); + return {db, cb}; }; + const db1 = enqueue(Polymer.Async.microTask); + const db2 = enqueue(Polymer.Async.microTask); + const db3 = enqueue(Polymer.Async.timeOut); + const db4 = enqueue(Polymer.Async.microTask); + enqueue(Polymer.Async.microTask, db2); + enqueue(Polymer.Async.microTask, db1); if (shouldFlush) { - expectedCallbacks = [timeoutCallback, ...microtaskCallbacks, ...nestedCallbacks]; Polymer.flush(); - // When flushing, order is order of enqueing - verify(); + assert.deepEqual(actualOrder, [db1.cb, db2.cb, db3.cb, db4.cb]); + done(); } else { - expectedCallbacks = [...microtaskCallbacks, ...nestedCallbacks, timeoutCallback]; - Polymer.Async.timeOut.run(verify); + Polymer.Async.timeOut.run(() => { + assert.deepEqual(actualOrder, [db4.cb, db2.cb, db1.cb, db3.cb]); + done(); + }); } - } + }; test('non-flushed', function(done) { testEnqueue(false, done); From 33c37fb8936c9906bbe6a6accd8f515a5ca59699 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 27 Feb 2019 18:17:52 -0800 Subject: [PATCH 6/8] Fix lint, update types --- test/unit/debounce.html | 1 - types/lib/utils/debounce.d.ts | 5 +++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/test/unit/debounce.html b/test/unit/debounce.html index fbb330753c..b380a62185 100644 --- a/test/unit/debounce.html +++ b/test/unit/debounce.html @@ -214,7 +214,6 @@ const testEnqueue = (shouldFlush, done) => { const actualOrder = []; - let i=1; const enqueue = (type, {db, cb} = {}) => { cb = cb || (() => actualOrder.push(cb)); db = Polymer.Debouncer.debounce(db, type, cb); diff --git a/types/lib/utils/debounce.d.ts b/types/lib/utils/debounce.d.ts index ceb853ef08..57bf1846ac 100644 --- a/types/lib/utils/debounce.d.ts +++ b/types/lib/utils/debounce.d.ts @@ -68,6 +68,11 @@ declare namespace Polymer { */ cancel(): void; + /** + * Cancels a debouncer's async callback. + */ + _cancelAsync(): void; + /** * Flushes an active debouncer and returns a reference to itself. */ From d06334e15133de40895dda6639955d162339a3d0 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 27 Feb 2019 18:31:51 -0800 Subject: [PATCH 7/8] Add comment about order when re-debouncing --- lib/utils/debounce.html | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/utils/debounce.html b/lib/utils/debounce.html index 01142cbd95..5b9e4d2a82 100644 --- a/lib/utils/debounce.html +++ b/lib/utils/debounce.html @@ -52,6 +52,10 @@ if (this.isActive()) { this._cancelAsync(); this._timer = null; + // Canceling a debouncer removes its spot from the flush queue, + // so if a debouncer is manually canceled and re-debounced, it + // will reset its flush order (this is a very minor difference from 1.x) + // Re-debouncing via the `debounce` API retains the 1.x FIFO flush order debouncerQueue.delete(this); } } From 2228f6787cde3096273ae9d5f5da7e1cc8720fea Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Thu, 14 Mar 2019 15:45:53 -0700 Subject: [PATCH 8/8] Sync with fixes on master branch. --- lib/utils/debounce.html | 9 ++-- test/unit/debounce.html | 117 +++++++++++++++++++++++++++------------- 2 files changed, 85 insertions(+), 41 deletions(-) diff --git a/lib/utils/debounce.html b/lib/utils/debounce.html index 5b9e4d2a82..5290285e3b 100644 --- a/lib/utils/debounce.html +++ b/lib/utils/debounce.html @@ -39,8 +39,8 @@ this._callback = callback; this._timer = this._asyncModule.run(() => { this._timer = null; - this._callback(); debouncerQueue.delete(this); + this._callback(); }); } /** @@ -51,7 +51,6 @@ cancel() { if (this.isActive()) { this._cancelAsync(); - this._timer = null; // Canceling a debouncer removes its spot from the flush queue, // so if a debouncer is manually canceled and re-debounced, it // will reset its flush order (this is a very minor difference from 1.x) @@ -65,7 +64,10 @@ * @return {void} */ _cancelAsync() { - this._asyncModule.cancel(/** @type {number} */(this._timer)); + if (this.isActive()) { + this._asyncModule.cancel(/** @type {number} */(this._timer)); + this._timer = null; + } } /** * Flushes an active debouncer and returns a reference to itself. @@ -158,7 +160,6 @@ }); } }); - debouncerQueue = new Set(); return didFlush; }; diff --git a/test/unit/debounce.html b/test/unit/debounce.html index b380a62185..c9fb0195ea 100644 --- a/test/unit/debounce.html +++ b/test/unit/debounce.html @@ -34,6 +34,86 @@ Polymer({is: 'x-basic'}); }); + suite('enqueueDebouncer & flush', function() { + + // NOTE: This is a regression test; the bug it fixed only occurred if the + // debouncer was flushed before any microtasks run, hence it should be + // first in this file + test('re-enqueue canceled debouncer', function() { + const cb = sinon.spy(); + let db; + db = Polymer.Debouncer.debounce(null, Polymer.Async.microTask, cb); + Polymer.enqueueDebouncer(db); + db.cancel(); + assert.equal(db.isActive(), false); + assert.equal(cb.callCount, 0); + db = Polymer.Debouncer.debounce(db, Polymer.Async.microTask, cb); + Polymer.enqueueDebouncer(db); + Polymer.flush(); + assert.isTrue(cb.calledOnce); + }); + + test('flushDebouncers from enqueued debouncer', function(done) { + const cb = sinon.spy(() => Polymer.flush()); + let db = Polymer.Debouncer.debounce(null, Polymer.Async.microTask, cb); + Polymer.enqueueDebouncer(db); + setTimeout(() => { + assert.isTrue(cb.calledOnce); + done(); + }); + }); + + const testEnqueue = (shouldFlush, done) => { + const actualOrder = []; + const enqueue = (type, {db, cb} = {}) => { + cb = cb || (() => actualOrder.push(cb)); + db = Polymer.Debouncer.debounce(db, type, cb); + Polymer.enqueueDebouncer(db); + return {db, cb}; + }; + const db1 = enqueue(Polymer.Async.microTask); + const db2 = enqueue(Polymer.Async.microTask); + const db3 = enqueue(Polymer.Async.timeOut); + const db4 = enqueue(Polymer.Async.microTask); + enqueue(Polymer.Async.microTask, db2); + enqueue(Polymer.Async.microTask, db1); + if (shouldFlush) { + Polymer.flush(); + assert.deepEqual(actualOrder, [db1.cb, db2.cb, db3.cb, db4.cb]); + done(); + } else { + Polymer.Async.timeOut.run(() => { + assert.deepEqual(actualOrder, [db4.cb, db2.cb, db1.cb, db3.cb]); + done(); + }); + } + }; + + test('non-flushed', function(done) { + testEnqueue(false, done); + }); + + test('flushed', function(done) { + testEnqueue(true, done); + }); + + test('reentrant flush', function() { + const cb2 = sinon.spy(); + let db2; + const cb1 = sinon.spy(() => { + Polymer.flush(); + db2 = Polymer.Debouncer.debounce(null, Polymer.Async.microTask, cb2); + Polymer.enqueueDebouncer(db2); + }); + const db1 = Polymer.Debouncer.debounce(null, Polymer.Async.microTask, cb1); + Polymer.enqueueDebouncer(db1); + Polymer.flush(); + assert.isTrue(cb1.calledOnce); + assert.isTrue(cb2.calledOnce); + }); + + }); + suite('debounce', function() { var element; @@ -208,43 +288,6 @@ }); }); - }); - - suite('enqueueDebouncer & flush', function() { - - const testEnqueue = (shouldFlush, done) => { - const actualOrder = []; - const enqueue = (type, {db, cb} = {}) => { - cb = cb || (() => actualOrder.push(cb)); - db = Polymer.Debouncer.debounce(db, type, cb); - Polymer.enqueueDebouncer(db); - return {db, cb}; - }; - const db1 = enqueue(Polymer.Async.microTask); - const db2 = enqueue(Polymer.Async.microTask); - const db3 = enqueue(Polymer.Async.timeOut); - const db4 = enqueue(Polymer.Async.microTask); - enqueue(Polymer.Async.microTask, db2); - enqueue(Polymer.Async.microTask, db1); - if (shouldFlush) { - Polymer.flush(); - assert.deepEqual(actualOrder, [db1.cb, db2.cb, db3.cb, db4.cb]); - done(); - } else { - Polymer.Async.timeOut.run(() => { - assert.deepEqual(actualOrder, [db4.cb, db2.cb, db1.cb, db3.cb]); - done(); - }); - } - }; - - test('non-flushed', function(done) { - testEnqueue(false, done); - }); - - test('flushed', function(done) { - testEnqueue(true, done); - }); });