From 60f6ccfb7dc9ce80f9a69e120ff3964b18b6605a Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Mon, 2 Mar 2020 22:44:19 -0800 Subject: [PATCH 1/9] Ensure limit is reset when initialCount is disabled. Note that any falsey value for initialCount (including `0`) is interpreted as "chunking disabled". This is consistent with 1.x logic, and follows from the logic of "starting chunking by rendering zero items" doesn't really make sense. --- lib/elements/dom-repeat.js | 26 +++-- test/unit/dom-repeat.html | 213 +++++++++++++++++++++++-------------- 2 files changed, 150 insertions(+), 89 deletions(-) diff --git a/lib/elements/dom-repeat.js b/lib/elements/dom-repeat.js index 0a88c81d0f..ef920fad84 100644 --- a/lib/elements/dom-repeat.js +++ b/lib/elements/dom-repeat.js @@ -245,15 +245,16 @@ export class DomRepeat extends domRepeatBase { }, /** - * Defines an initial count of template instances to render after setting - * the `items` array, before the next paint, and puts the `dom-repeat` - * into "chunking mode". The remaining items (and any future items as a - * result of pushing onto the array) will be created and rendered - * incrementally at each animation frame thereof until all instances have - * been rendered. + * When greater than zero, defines an initial count of template instances + * to render after setting the `items` array, before the next paint, and + * puts the `dom-repeat` into "chunking mode". The remaining items (and + * any future items as a result of pushing onto the array) will be created + * and rendered incrementally at each animation frame thereof until all + * instances have been rendered. */ initialCount: { - type: Number + type: Number, + observer: '__initialCountChanged' }, /** @@ -471,6 +472,17 @@ export class DomRepeat extends domRepeatBase { this.observe.replace('.*', '.').split(' '); } + __initialCountChanged(value) { + // When not chunking, ensure the list is unlimited + if (!value) { + this.__limit = Infinity; + } + // When chunking, `__limit` is managed in `__updateLimit` called during + // `__render`, which ensures the limit takes into account initialCount, the + // filtered items length, and any increases based on the throttled + // __chunkCount + } + __handleObservedPaths(path) { // Handle cases where path changes should cause a re-sort/filter if (this.__sortFn || this.__filterFn) { diff --git a/test/unit/dom-repeat.html b/test/unit/dom-repeat.html index 9dce86e49d..18390dea76 100644 --- a/test/unit/dom-repeat.html +++ b/test/unit/dom-repeat.html @@ -3945,28 +3945,38 @@

x-repeat-limit

}); -suite('chunked rendering', function() { +suite.only('chunked rendering', function() { let chunked; - let verifyAfterChange; - const verify = () => verifyAfterChange && verifyAfterChange(); + let repeat; + let resolve; + let targetCount; + const handleChange = () => { + if (!targetCount || chunked.$.repeater.renderedItemCount >= targetCount) { + resolve(Array.from(chunked.root.querySelectorAll('*:not(template):not(dom-repeat)'))); + } + }; + const waitUntilRendered = async (count) => { + targetCount = count; + return await new Promise(r => resolve = r); + }; setup(() => { chunked = document.createElement('x-repeat-chunked'); - chunked.addEventListener('dom-change', verify); + chunked.addEventListener('dom-change', handleChange); document.body.appendChild(chunked); + repeat = chunked.$.repeat; }); teardown(() => { - chunked.removeEventListener('dom-change', verify); + chunked.removeEventListener('dom-change', handleChange); document.body.removeChild(chunked); chunked = null; - verifyAfterChange = null; }); // Framerate=25, element cost = 4ms: should never make more than // (1000/25) / 4 = 10 elements per frame const MAX_PER_FRAME = (1000 / 25) / 4; - test('basic chunked rendering', function(done) { + test('basic chunked rendering', async () => { var checkItemOrder = function(stamped) { for (var i=0; ix-repeat-limit } }; + // Set items to chunk + chunked.items = chunked.preppedItems.slice(); + + let stamped = []; let lastStamped; let frameCount = 0; - verifyAfterChange = function() { - var stamped = Array.from(chunked.root.querySelectorAll('*:not(template):not(dom-repeat)')); + // Loop until chunking is complete + while (stamped.length < chunked.items.length) { + frameCount++; + stamped = await waitUntilRendered(); checkItemOrder(stamped); if (!lastStamped) { // Initial rendering of initial count @@ -3991,21 +4007,14 @@

x-repeat-limit

assert.isAtMost((stamped.length - lastStamped.length), MAX_PER_FRAME, `list should not render more than ${MAX_PER_FRAME} per frame`); } - if (stamped.length < chunked.items.length) { - frameCount++; - lastStamped = stamped; - } else { - // Final rendering at exact item count - assert.equal(stamped.length, 100, 'final count wrong'); - assert.isAtLeast(frameCount, 10, 'should have taken at least 10 frames to render'); - done(); - } - }; - chunked.items = chunked.preppedItems.slice(); - + lastStamped = stamped; + } + // Final rendering at exact item count + assert.equal(stamped.length, 100, 'final count wrong'); + assert.isAtLeast(frameCount, 10, 'should have taken at least 10 frames to render'); }); - test('mutations during chunked rendering', function(done) { + test('mutations during chunked rendering', async () => { var checkItemOrder = function(stamped) { var last = -1; @@ -4038,10 +4047,15 @@

x-repeat-limit

} }; + // Set items to chunk + chunked.items = chunked.preppedItems.slice(); + + let stamped = []; let lastStamped; - var frameCount = 0; - verifyAfterChange = function() { - var stamped = Array.from(chunked.root.querySelectorAll('*:not(template):not(dom-repeat)')); + let frameCount = 0; + // Loop until chunking is complete + while (stamped.length < chunked.items.length) { + stamped = await waitUntilRendered(); checkItemOrder(stamped); if (!lastStamped) { // Initial rendering of initial count @@ -4055,24 +4069,20 @@

x-repeat-limit

assert.isAtMost((stamped.length - lastStamped.length), MAX_PER_FRAME, `list should not render more than ${MAX_PER_FRAME} per frame`); } - if (stamped.length < chunked.items.length) { - if (frameCount++ < 5) { - mutateArray(chunked, stamped); - } - lastStamped = stamped; - } else { - // Final rendering at exact item count - assert.equal(stamped.length, chunked.items.length, 'final count wrong'); - assert.isAtLeast(frameCount, 10, 'should have taken at least 10 frames to render'); - done(); + if (stamped.length < chunked.items.length && frameCount < 5) { + mutateArray(chunked, stamped); } - }; - chunked.items = chunked.preppedItems.slice(); + lastStamped = stamped; + frameCount++; + } + // Final rendering at exact item count + assert.equal(stamped.length, chunked.items.length, 'final count wrong'); + assert.isAtLeast(frameCount, 10, 'should have taken at least 10 frames to render'); }); - test('mutations during chunked rendering, sort & filtered', function(done) { + test('mutations during chunked rendering, sort & filtered', async () => { var checkItemOrder = function(stamped) { var last = Infinity; @@ -4094,12 +4104,23 @@

x-repeat-limit

chunked.splice('items', Math.round(stamped.length/2), 3); }; - var lastStamped; - var frameCount = 0; - verifyAfterChange = function() { - var stamped = Array.from(chunked.root.querySelectorAll('*:not(template):not(dom-repeat)')); + // Set items to chunk + chunked.$.repeater.sort = function(a, b) { + return b.prop - a.prop; + }; + chunked.$.repeater.filter = function(a) { + return (a.prop % 2) === 0; + }; + chunked.items = chunked.preppedItems.slice(); + + let stamped = []; + let lastStamped; + let frameCount = 0; + let filteredLength = chunked.items.filter(chunked.$.repeater.filter).length; + // Loop until chunking is complete + while (stamped.length < filteredLength) { + stamped = await waitUntilRendered(); checkItemOrder(stamped); - var filteredLength = chunked.items.filter(chunked.$.repeater.filter).length; if (!lastStamped) { // Initial rendering of initial count assert.equal(stamped.length, 10); @@ -4112,37 +4133,35 @@

x-repeat-limit

assert.isAtMost((stamped.length - lastStamped.length), MAX_PER_FRAME, `list should not render more than ${MAX_PER_FRAME} per frame`); } - if (stamped.length < filteredLength) { - if (frameCount++ < 4) { - mutateArray(chunked, stamped); - } - lastStamped = stamped; - } else { - assert.equal(stamped.length, filteredLength, 'final count wrong'); - assert.isAtLeast(frameCount, 5, 'should have taken at least 5 frames to render'); - done(); + if (stamped.length < chunked.items.length && frameCount < 4) { + mutateArray(chunked, stamped); + filteredLength = chunked.items.filter(chunked.$.repeater.filter).length; } - }; - chunked.$.repeater.sort = function(a, b) { - return b.prop - a.prop; - }; - chunked.$.repeater.filter = function(a) { - return (a.prop % 2) === 0; - }; - chunked.items = chunked.preppedItems.slice(); - + lastStamped = stamped; + frameCount++; + } + // Final rendering at exact item count + assert.equal(stamped.length, filteredLength, 'final count wrong'); + assert.isAtLeast(frameCount, 5, 'should have taken at least 5 frames to render'); }); suite('resetting items array', () => { [false, true].forEach(reuse => { - test(`reuseChunkedInstances=${reuse}`, (done) => { + test(`reuseChunkedInstances=${reuse}`, async () => { - let i = 3; + // Set items to chunk + chunked.$.repeater.reuseChunkedInstances = reuse; + chunked.items = chunked.preppedItems.slice(); + + let resetCount = 3; + let stamped = []; let lastStamped; - verifyAfterChange = function() { - var stamped = Array.from(chunked.root.querySelectorAll('*:not(template):not(dom-repeat)')); + let frameCount = 0; + // Loop until chunking is complete + while (stamped.length < chunked.items.length) { + stamped = await waitUntilRendered(); if (!lastStamped) { // Initial rendering of initial count assert.equal(stamped.length, 10); @@ -4155,30 +4174,61 @@

x-repeat-limit

assert.isAtMost((stamped.length - lastStamped.length), MAX_PER_FRAME, `list should not render more than ${MAX_PER_FRAME} per frame`); } - if (stamped.length < chunked.items.length) { - lastStamped = stamped; - } else { - assert.equal(stamped.length, chunked.items.length, 'final count wrong'); - if (--i > 0) { - if (!reuse) { - lastStamped = null; - } - chunked.items = chunked.preppedItems.slice(); - } else { - done(); + lastStamped = stamped; + frameCount++; + if (--resetCount > 0) { + if (!reuse) { + lastStamped = null; } + chunked.items = chunked.preppedItems.slice(); } - }; - - chunked.$.repeater.reuseChunkedInstances = reuse; - chunked.items = chunked.preppedItems.slice(); - + } + // Final rendering at exact item count + assert.equal(stamped.length, chunked.items.length, 'final count wrong'); + assert.isAtLeast(frameCount, 5, 'should have taken at least 5 frames to render'); + }); }); }); + test('changing to/from initialCount=0', async () => { + // Render all + chunked.items = chunked.preppedItems.slice(); + let stamped = await waitUntilRendered(chunked.preppedItems.length); + assert.equal(stamped.length, chunked.preppedItems.length); + // Clear the list + chunked.items = []; + stamped = await waitUntilRendered(0); + assert.equal(stamped.length, 0); + // Disable chunking + chunked.$.repeater.initialCount = 0; + // Render all + chunked.items = chunked.preppedItems.slice(); + stamped = await waitUntilRendered(chunked.preppedItems.length); + assert.equal(stamped.length, chunked.preppedItems.length); + // Clear the list + chunked.items = []; + stamped = await waitUntilRendered(0); + assert.equal(stamped.length, 0); + // Re-enable chunking + chunked.$.repeater.initialCount = 10; + // Render all; the initial render should have the initial count, and then + // chunk until the end of the list + let frameCount = 0; + chunked.items = chunked.preppedItems.slice(); + while (stamped.length < chunked.items.length) { + stamped = await waitUntilRendered(10); + if (frameCount === 0) { + assert.equal(stamped.length, 10); + } + frameCount++; + } + assert.equal(stamped.length, chunked.preppedItems.length); + assert.isAtLeast(frameCount, 2); + }); + }); suite('misc', function() { @@ -4233,7 +4283,6 @@

x-repeat-limit

test('paths update on observed properties', function() { let simple = fixture('simple'); flush(); - //debugger; var stamped = simple.root.querySelectorAll('*:not(template):not(dom-repeat)'); assert.equal(stamped[0].itemaProp, 'prop-1'); simple.$.repeat.observe = 'prop'; From b503db15f185b02158f3637e8c32cc095e9928f3 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Mon, 2 Mar 2020 22:51:52 -0800 Subject: [PATCH 2/9] Remove accidental commit of suite.only --- test/unit/dom-repeat.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/dom-repeat.html b/test/unit/dom-repeat.html index 18390dea76..fa466b8b95 100644 --- a/test/unit/dom-repeat.html +++ b/test/unit/dom-repeat.html @@ -3945,7 +3945,7 @@

x-repeat-limit

}); -suite.only('chunked rendering', function() { +suite('chunked rendering', function() { let chunked; let repeat; From 1d96db3c387a5276d40a7021593ab3ca46e8e90b Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Mon, 2 Mar 2020 23:41:12 -0800 Subject: [PATCH 3/9] When re-enabling, ensure __limit is at a good starting point and add a test for that. Also: * Ensure `__itemsArrayChanged` is cleared after every render. * Enqueue `__continueChunkingAfterRaf` before notifying renderedItemCount for safety --- lib/elements/dom-repeat.js | 69 ++++++++++++++++++-------------------- test/unit/dom-repeat.html | 23 ++++++++++--- 2 files changed, 51 insertions(+), 41 deletions(-) diff --git a/lib/elements/dom-repeat.js b/lib/elements/dom-repeat.js index ef920fad84..6240f982d2 100644 --- a/lib/elements/dom-repeat.js +++ b/lib/elements/dom-repeat.js @@ -472,15 +472,10 @@ export class DomRepeat extends domRepeatBase { this.observe.replace('.*', '.').split(' '); } - __initialCountChanged(value) { - // When not chunking, ensure the list is unlimited - if (!value) { - this.__limit = Infinity; - } - // When chunking, `__limit` is managed in `__updateLimit` called during - // `__render`, which ensures the limit takes into account initialCount, the - // filtered items length, and any increases based on the throttled - // __chunkCount + __initialCountChanged(initialCount) { + // When enabling chunking, start the limit at the current rendered count, + // otherwie ensure the list is unlimited + this.__limit = initialCount ? this.renderedItemCount : Infinity; } __handleObservedPaths(path) { @@ -554,18 +549,16 @@ export class DomRepeat extends domRepeatBase { const isntIdxToItemsIdx = this.__sortAndFilterItems(items); // If we're chunking, increase the limit if there are new instances to // create and schedule the next chunk - if (this.initialCount) { - this.__updateLimit(isntIdxToItemsIdx.length); - } + this.__updateLimit(isntIdxToItemsIdx.length); // Create, update, and/or remove instances this.__updateInstances(items, isntIdxToItemsIdx); - // Set rendered item count - this._setRenderedItemCount(this.__instances.length); // If we're chunking, schedule a rAF task to measure/continue chunking if (this.initialCount && (this.__shouldMeasureChunk || this.__shouldContinueChunking)) { this.__debounceRender(this.__continueChunkingAfterRaf); } + // Set rendered item count + this._setRenderedItemCount(this.__instances.length); // Notify users if (!suppressTemplateNotifications || this.notifyDomChange) { this.dispatchEvent(new CustomEvent('dom-change', { @@ -594,31 +587,33 @@ export class DomRepeat extends domRepeatBase { } __updateLimit(filteredItemCount) { - let newCount; - if (!this.__chunkCount || - (this.__itemsArrayChanged && !this.reuseChunkedInstances)) { - // Limit next render to the initial count - this.__limit = Math.min(filteredItemCount, this.initialCount); - // Subtract off any existing instances to determine the number of - // instances that will be created - newCount = Math.max(this.__limit - this.__instances.length, 0); - // Initialize the chunk size with how many items we're creating - this.__chunkCount = newCount || 1; - } else { - // The number of new instances that will be created is based on the - // existing instances, the new list size, and the maximum chunk size - newCount = Math.min( - Math.max(filteredItemCount - this.__instances.length, 0), - this.__chunkCount); - // Update the limit based on how many new items we're making, limited - // buy the total size of the list - this.__limit = Math.min(this.__limit + newCount, filteredItemCount); + if (this.initialCount) { + let newCount; + if (!this.__chunkCount || + (this.__itemsArrayChanged && !this.reuseChunkedInstances)) { + // Limit next render to the initial count + this.__limit = Math.min(filteredItemCount, this.initialCount); + // Subtract off any existing instances to determine the number of + // instances that will be created + newCount = Math.max(this.__limit - this.__instances.length, 0); + // Initialize the chunk size with how many items we're creating + this.__chunkCount = newCount || 1; + } else { + // The number of new instances that will be created is based on the + // existing instances, the new list size, and the maximum chunk size + newCount = Math.min( + Math.max(filteredItemCount - this.__instances.length, 0), + this.__chunkCount); + // Update the limit based on how many new items we're making, limited + // buy the total size of the list + this.__limit = Math.min(this.__limit + newCount, filteredItemCount); + } + // Record some state about chunking for use in `__continueChunking` + this.__shouldMeasureChunk = newCount === this.__chunkCount; + this.__shouldContinueChunking = this.__limit < filteredItemCount; + this.__renderStartTime = performance.now(); } this.__itemsArrayChanged = false; - // Record some state about chunking for use in `__continueChunking` - this.__shouldMeasureChunk = newCount === this.__chunkCount; - this.__shouldContinueChunking = this.__limit < filteredItemCount; - this.__renderStartTime = performance.now(); } __continueChunkingAfterRaf() { diff --git a/test/unit/dom-repeat.html b/test/unit/dom-repeat.html index fa466b8b95..e6edd8f92c 100644 --- a/test/unit/dom-repeat.html +++ b/test/unit/dom-repeat.html @@ -3948,7 +3948,6 @@

x-repeat-limit

suite('chunked rendering', function() { let chunked; - let repeat; let resolve; let targetCount; const handleChange = () => { @@ -3964,7 +3963,6 @@

x-repeat-limit

chunked = document.createElement('x-repeat-chunked'); chunked.addEventListener('dom-change', handleChange); document.body.appendChild(chunked); - repeat = chunked.$.repeat; }); teardown(() => { chunked.removeEventListener('dom-change', handleChange); @@ -4193,7 +4191,7 @@

x-repeat-limit

}); - test('changing to/from initialCount=0', async () => { + test.only('changing to/from initialCount=0', async () => { // Render all chunked.items = chunked.preppedItems.slice(); let stamped = await waitUntilRendered(chunked.preppedItems.length); @@ -4219,7 +4217,7 @@

x-repeat-limit

let frameCount = 0; chunked.items = chunked.preppedItems.slice(); while (stamped.length < chunked.items.length) { - stamped = await waitUntilRendered(10); + stamped = await waitUntilRendered(); if (frameCount === 0) { assert.equal(stamped.length, 10); } @@ -4227,6 +4225,23 @@

x-repeat-limit

} assert.equal(stamped.length, chunked.preppedItems.length); assert.isAtLeast(frameCount, 2); + // Disable chunking + chunked.$.repeater.initialCount = 0; + // Render some + chunked.items = chunked.preppedItems.slice(0, 20); + stamped = await waitUntilRendered(); + assert.equal(stamped.length, chunked.items.length); + // Re-enable chunking + chunked.$.repeater.initialCount = 10; + // Push remaining; these should chunk out + frameCount = 0; + chunked.push('items', ...chunked.preppedItems.slice(20)); + while (stamped.length < chunked.items.length) { + stamped = await waitUntilRendered(); + frameCount++; + } + assert.equal(stamped.length, chunked.items.length); + assert.isAtLeast(frameCount, 2); }); }); From d67a8b5192a2d998fac1f24d363020bf3e4502dd Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Mon, 2 Mar 2020 23:45:50 -0800 Subject: [PATCH 4/9] Remove accidental commit of test.only --- test/unit/dom-repeat.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/dom-repeat.html b/test/unit/dom-repeat.html index e6edd8f92c..b0292dda21 100644 --- a/test/unit/dom-repeat.html +++ b/test/unit/dom-repeat.html @@ -4191,7 +4191,7 @@

x-repeat-limit

}); - test.only('changing to/from initialCount=0', async () => { + test('changing to/from initialCount=0', async () => { // Render all chunked.items = chunked.preppedItems.slice(); let stamped = await waitUntilRendered(chunked.preppedItems.length); From a02ed026e327b3832d35d4be0350a7ccce11ba41 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Tue, 3 Mar 2020 17:17:34 -0800 Subject: [PATCH 5/9] Update Sauce config to drop Safari 9, add 12 & 13. Safari 9 is now very old, and has micro task ordering bugs issues that make testing flaky. --- util/travis-sauce-test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/travis-sauce-test.sh b/util/travis-sauce-test.sh index c2f80dd32b..ee4eb92a61 100755 --- a/util/travis-sauce-test.sh +++ b/util/travis-sauce-test.sh @@ -9,4 +9,4 @@ # subject to an additional IP rights grant found at http://polymer.github.io/PATENTS.txt # set -x -node ./node_modules/.bin/polymer test --npm --module-resolution=node -s 'windows 10/microsoftedge@15' -s 'windows 10/microsoftedge@17' -s 'windows 8.1/internet explorer@11' -s 'os x 10.11/safari@9' -s 'macos 10.12/safari@10' -s 'macos 10.13/safari@11' -s 'Linux/chrome@41' \ No newline at end of file +node ./node_modules/.bin/polymer test --npm --module-resolution=node -s 'windows 10/microsoftedge@15' -s 'windows 10/microsoftedge@17' -s 'windows 8.1/internet explorer@11' -s 'macos 10.13/safari@11' -s 'macos 10.13/safari@12' -s 'macos 10.13/safari@13' -s 'Linux/chrome@41' \ No newline at end of file From b5664cba107c464c6b2c7588756580075891e72c Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 4 Mar 2020 11:20:57 -0800 Subject: [PATCH 6/9] Simplify by making limit a derived value from existing state. This centralizes the calculation of limit based on changes to other state variables. --- lib/elements/dom-repeat.js | 40 ++++++++++++++---------------- test/unit/dom-repeat.html | 51 ++++++++++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 30 deletions(-) diff --git a/lib/elements/dom-repeat.js b/lib/elements/dom-repeat.js index 6240f982d2..7b2d1dbee2 100644 --- a/lib/elements/dom-repeat.js +++ b/lib/elements/dom-repeat.js @@ -253,8 +253,7 @@ export class DomRepeat extends domRepeatBase { * instances have been rendered. */ initialCount: { - type: Number, - observer: '__initialCountChanged' + type: Number }, /** @@ -318,7 +317,6 @@ export class DomRepeat extends domRepeatBase { constructor() { super(); this.__instances = []; - this.__limit = Infinity; this.__renderDebouncer = null; this.__itemsIdxToInstIdx = {}; this.__chunkCount = null; @@ -472,12 +470,6 @@ export class DomRepeat extends domRepeatBase { this.observe.replace('.*', '.').split(' '); } - __initialCountChanged(initialCount) { - // When enabling chunking, start the limit at the current rendered count, - // otherwie ensure the list is unlimited - this.__limit = initialCount ? this.renderedItemCount : Infinity; - } - __handleObservedPaths(path) { // Handle cases where path changes should cause a re-sort/filter if (this.__sortFn || this.__filterFn) { @@ -549,9 +541,9 @@ export class DomRepeat extends domRepeatBase { const isntIdxToItemsIdx = this.__sortAndFilterItems(items); // If we're chunking, increase the limit if there are new instances to // create and schedule the next chunk - this.__updateLimit(isntIdxToItemsIdx.length); + const limit = this.__calculateLimit(isntIdxToItemsIdx.length); // Create, update, and/or remove instances - this.__updateInstances(items, isntIdxToItemsIdx); + this.__updateInstances(items, limit, isntIdxToItemsIdx); // If we're chunking, schedule a rAF task to measure/continue chunking if (this.initialCount && (this.__shouldMeasureChunk || this.__shouldContinueChunking)) { @@ -586,34 +578,39 @@ export class DomRepeat extends domRepeatBase { return isntIdxToItemsIdx; } - __updateLimit(filteredItemCount) { + __calculateLimit(filteredItemCount) { + let limit = filteredItemCount; + // When chunking, we increase the limit from the currently rendered count + // by the chunk count that is re-calculated after each rAF (with special + // cases for reseting the limit to initialCount after changing items) if (this.initialCount) { let newCount; if (!this.__chunkCount || (this.__itemsArrayChanged && !this.reuseChunkedInstances)) { // Limit next render to the initial count - this.__limit = Math.min(filteredItemCount, this.initialCount); + limit = Math.min(filteredItemCount, this.initialCount); // Subtract off any existing instances to determine the number of // instances that will be created - newCount = Math.max(this.__limit - this.__instances.length, 0); + newCount = Math.max(limit - this.renderedItemCount, 0); // Initialize the chunk size with how many items we're creating this.__chunkCount = newCount || 1; } else { // The number of new instances that will be created is based on the - // existing instances, the new list size, and the maximum chunk size + // existing instances, the new list size, and the chunk size newCount = Math.min( - Math.max(filteredItemCount - this.__instances.length, 0), + Math.max(filteredItemCount - this.renderedItemCount, 0), this.__chunkCount); // Update the limit based on how many new items we're making, limited // buy the total size of the list - this.__limit = Math.min(this.__limit + newCount, filteredItemCount); + limit = Math.min(this.renderedItemCount + newCount, filteredItemCount); } // Record some state about chunking for use in `__continueChunking` this.__shouldMeasureChunk = newCount === this.__chunkCount; - this.__shouldContinueChunking = this.__limit < filteredItemCount; + this.__shouldContinueChunking = limit < filteredItemCount; this.__renderStartTime = performance.now(); } this.__itemsArrayChanged = false; + return limit; } __continueChunkingAfterRaf() { @@ -638,13 +635,12 @@ export class DomRepeat extends domRepeatBase { } } - __updateInstances(items, isntIdxToItemsIdx) { + __updateInstances(items, limit, isntIdxToItemsIdx) { // items->inst map kept for item path forwarding const itemsIdxToInstIdx = this.__itemsIdxToInstIdx = {}; - let instIdx = 0; + let instIdx; // Generate instances and assign items - const limit = Math.min(isntIdxToItemsIdx.length, this.__limit); - for (; instIdxx-repeat-limit suite('limit', function() { + // These tests verify correct operation of e.g. path notifications for a + // pertially rendered list; it does this by monkey-patching the internal + // `__calculateLimit` method to force the list to a certain length. + // While they rely on some internal implementation details, the tests are + // done this way because getting the list in a certain known state + // during live chunking is tricky due to the feedback-based throttling. + + suiteSetup(() => { + limited.$.repeater.__calculateLimit = function() { + return this.__limit; + } + }); + var checkItemOrder = function(stamped) { for (var i=0; ix-repeat-limit checkItemOrder(stamped); }); - test('increase limit above items.length', function() { - limited.$.repeater.__limit = 30; - limited.$.repeater.render(); - var stamped = limited.root.querySelectorAll('*:not(template):not(dom-repeat)'); - assert.equal(stamped.length, 20); - checkItemOrder(stamped); - }); - test('decrease limit', function() { // Decrease limit limited.$.repeater.__limit = 15; @@ -4010,6 +4015,36 @@

x-repeat-limit

// Final rendering at exact item count assert.equal(stamped.length, 100, 'final count wrong'); assert.isAtLeast(frameCount, 10, 'should have taken at least 10 frames to render'); + + // Set less than initial count, should trim list in one frame + chunked.items = chunked.preppedItems.slice(0, 5); + stamped = await waitUntilRendered(); + assert.equal(stamped.length, 5, 'final count wrong'); + assert.deepEqual(lastStamped.slice(0, stamped.length), stamped, + 'list should not re-render instances during mutation'); + lastStamped = stamped; + + // Set at initial count, should render in one frame + chunked.items = chunked.preppedItems.slice(0, 10); + stamped = await waitUntilRendered(); + assert.equal(stamped.length, 10, 'final count wrong'); + assert.deepEqual(lastStamped, stamped.slice(0, lastStamped.length), + 'list should not re-render instances during mutation'); + lastStamped = stamped; + + // Set over initial count, should render in more than one frame + chunked.items = chunked.preppedItems.slice(0, 10 + MAX_PER_FRAME * 2); + stamped = await waitUntilRendered(); + assert.deepEqual(lastStamped, stamped.slice(0, 10), + 'list should not re-render instances during mutation'); + frameCount = 0; + while (stamped.length < chunked.items.length) { + stamped = await waitUntilRendered(); + frameCount++; + } + assert.isAtLeast(frameCount, 2, 'should have taken at least 2 frames to render'); + assert.equal(stamped.length, chunked.items.length, 'final count wrong'); + }); test('mutations during chunked rendering', async () => { From 91f01e57819311dba811f037558cab11093ebdcf Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 4 Mar 2020 11:24:16 -0800 Subject: [PATCH 7/9] Remove obsolete tests. --- test/unit/dom-repeat.html | 486 ++++++++++++++++++-------------------- 1 file changed, 233 insertions(+), 253 deletions(-) diff --git a/test/unit/dom-repeat.html b/test/unit/dom-repeat.html index 30a6dad470..119149d412 100644 --- a/test/unit/dom-repeat.html +++ b/test/unit/dom-repeat.html @@ -3669,283 +3669,263 @@

x-repeat-limit

suiteSetup(() => { limited.$.repeater.__calculateLimit = function() { return this.__limit; - } + }; }); - var checkItemOrder = function(stamped) { - for (var i=0; i Date: Wed, 4 Mar 2020 11:33:19 -0800 Subject: [PATCH 8/9] Improve comment. --- lib/elements/dom-repeat.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/elements/dom-repeat.js b/lib/elements/dom-repeat.js index 7b2d1dbee2..27d2f79728 100644 --- a/lib/elements/dom-repeat.js +++ b/lib/elements/dom-repeat.js @@ -544,7 +544,10 @@ export class DomRepeat extends domRepeatBase { const limit = this.__calculateLimit(isntIdxToItemsIdx.length); // Create, update, and/or remove instances this.__updateInstances(items, limit, isntIdxToItemsIdx); - // If we're chunking, schedule a rAF task to measure/continue chunking + // If we're chunking, schedule a rAF task to measure/continue chunking. + // Do this before any notifying events (renderedItemCount & dom-change) + // since those could modify items and enqueue a new full render which will + // pre-empt this measurement. if (this.initialCount && (this.__shouldMeasureChunk || this.__shouldContinueChunking)) { this.__debounceRender(this.__continueChunkingAfterRaf); From ddb37df98593f1209bcc3e77d79850284aea2d85 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 4 Mar 2020 22:41:55 -0800 Subject: [PATCH 9/9] Ensure any previously enqueued rAF is canceled when re-rendering. Also, use instances length instead of renderedItemCount since it will be undefined on first render. --- lib/elements/dom-repeat.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/elements/dom-repeat.js b/lib/elements/dom-repeat.js index 27d2f79728..3a2c7d9175 100644 --- a/lib/elements/dom-repeat.js +++ b/lib/elements/dom-repeat.js @@ -324,6 +324,7 @@ export class DomRepeat extends domRepeatBase { this.__itemsArrayChanged = false; this.__shouldMeasureChunk = false; this.__shouldContinueChunking = false; + this.__chunkingId = 0; this.__sortFn = null; this.__filterFn = null; this.__observePaths = null; @@ -550,7 +551,8 @@ export class DomRepeat extends domRepeatBase { // pre-empt this measurement. if (this.initialCount && (this.__shouldMeasureChunk || this.__shouldContinueChunking)) { - this.__debounceRender(this.__continueChunkingAfterRaf); + cancelAnimationFrame(this.__chunkingId); + this.__chunkingId = requestAnimationFrame(() => this.__continueChunking()); } // Set rendered item count this._setRenderedItemCount(this.__instances.length); @@ -583,6 +585,7 @@ export class DomRepeat extends domRepeatBase { __calculateLimit(filteredItemCount) { let limit = filteredItemCount; + const currentCount = this.__instances.length; // When chunking, we increase the limit from the currently rendered count // by the chunk count that is re-calculated after each rAF (with special // cases for reseting the limit to initialCount after changing items) @@ -594,18 +597,18 @@ export class DomRepeat extends domRepeatBase { limit = Math.min(filteredItemCount, this.initialCount); // Subtract off any existing instances to determine the number of // instances that will be created - newCount = Math.max(limit - this.renderedItemCount, 0); + newCount = Math.max(limit - currentCount, 0); // Initialize the chunk size with how many items we're creating this.__chunkCount = newCount || 1; } else { // The number of new instances that will be created is based on the // existing instances, the new list size, and the chunk size newCount = Math.min( - Math.max(filteredItemCount - this.renderedItemCount, 0), + Math.max(filteredItemCount - currentCount, 0), this.__chunkCount); // Update the limit based on how many new items we're making, limited // buy the total size of the list - limit = Math.min(this.renderedItemCount + newCount, filteredItemCount); + limit = Math.min(currentCount + newCount, filteredItemCount); } // Record some state about chunking for use in `__continueChunking` this.__shouldMeasureChunk = newCount === this.__chunkCount; @@ -616,10 +619,6 @@ export class DomRepeat extends domRepeatBase { return limit; } - __continueChunkingAfterRaf() { - requestAnimationFrame(() => this.__continueChunking()); - } - __continueChunking() { // Simple auto chunkSize throttling algorithm based on feedback loop: // measure actual time between frames and scale chunk count by ratio of