Skip to content

Commit

Permalink
Simplify by making limit a derived value from existing state.
Browse files Browse the repository at this point in the history
This centralizes the calculation of limit based on changes to other state variables.
  • Loading branch information
kevinpschaaf committed Mar 4, 2020
1 parent a02ed02 commit b5664cb
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 30 deletions.
40 changes: 18 additions & 22 deletions lib/elements/dom-repeat.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,7 @@ export class DomRepeat extends domRepeatBase {
* instances have been rendered.
*/
initialCount: {
type: Number,
observer: '__initialCountChanged'
type: Number
},

/**
Expand Down Expand Up @@ -318,7 +317,6 @@ export class DomRepeat extends domRepeatBase {
constructor() {
super();
this.__instances = [];
this.__limit = Infinity;
this.__renderDebouncer = null;
this.__itemsIdxToInstIdx = {};
this.__chunkCount = null;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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() {
Expand All @@ -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 (; instIdx<limit; instIdx++) {
for (instIdx=0; instIdx<limit; instIdx++) {
let inst = this.__instances[instIdx];
let itemIdx = isntIdxToItemsIdx[instIdx];
let item = items[itemIdx];
Expand Down
51 changes: 43 additions & 8 deletions test/unit/dom-repeat.html
Original file line number Diff line number Diff line change
Expand Up @@ -3659,6 +3659,19 @@ <h4>x-repeat-limit</h4>

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; i<stamped.length; i++) {
assert.equal(parseInt(stamped[i].textContent), i);
Expand Down Expand Up @@ -3704,14 +3717,6 @@ <h4>x-repeat-limit</h4>
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;
Expand Down Expand Up @@ -4010,6 +4015,36 @@ <h4>x-repeat-limit</h4>
// 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 () => {
Expand Down

0 comments on commit b5664cb

Please sign in to comment.