Skip to content

Commit

Permalink
Revert "amp-list: Fix Bind.rescan vs. diffing race condition (ampproj…
Browse files Browse the repository at this point in the history
…ect#32650)"

This reverts commit a0f71df.
  • Loading branch information
samouri committed Mar 12, 2021
1 parent c8b2faf commit 497fe1d
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 89 deletions.
58 changes: 13 additions & 45 deletions extensions/amp-list/0.1/amp-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -1095,19 +1095,21 @@ export class AmpList extends AMP.BaseElement {

// binding=refresh: Only do render-blocking update after initial render.
if (binding && binding.startsWith('refresh')) {
// Don't bother using bindForDocOrNull() since the Bind service must be available after first mutate.
const afterFirstMutate =
this.bind_ && this.bind_.signals().get('FIRST_MUTATE');
if (afterFirstMutate) {
// Bind service must be available after first mutation, so don't
// wait on the async service getter.
if (this.bind_ && this.bind_.signals().get('FIRST_MUTATE')) {
return updateWith(this.bind_);
} else {
// This must be initial render, so do a non-blocking scan for bindings only.
// [diffable] is a special case that is handled later in render_(), see comment there.
if (!this.element.hasAttribute('diffable')) {
this.scanForBindings_(elements, []);
}

// Don't block render and return synchronously.
// On initial render, do a non-blocking scan and don't update.
Services.bindForDocOrNull(this.element).then((bind) => {
if (bind) {
const evaluate = binding == 'refresh-evaluate';
bind.rescan(elements, [], {
'fast': true,
'update': evaluate ? 'evaluate' : false,
});
}
});
return Promise.resolve(elements);
}
}
Expand All @@ -1122,32 +1124,6 @@ export class AmpList extends AMP.BaseElement {
});
}

/**
* Scans for bindings in `addedElements` and removes bindings in `removedElements`.
* Unlike updateBindings(), does NOT apply bindings or update DOM.
* Should only be used for binding="refresh" or binding="refresh-evaluate".
* @param {!Array<!Element>} addedElements
* @param {!Array<!Element>} removedElements
* @private
*/
scanForBindings_(addedElements, removedElements) {
const binding = this.element.getAttribute('binding');
if (!binding || !binding.startsWith('refresh')) {
return;
}
Services.bindForDocOrNull(this.element).then((bind) => {
if (bind) {
// For binding="refresh-evaluate", we scan for bindings, evaluate+cache expressions, but skip DOM update.
// For binding="refresh", we only scan for bindings.
const update = binding == 'refresh-evaluate' ? 'evaluate' : false;
bind.rescan(addedElements, removedElements, {
'fast': true,
'update': update,
});
}
});
}

/**
* @param {!Array<!Element>} elements
* @param {boolean=} opt_append
Expand All @@ -1164,14 +1140,6 @@ export class AmpList extends AMP.BaseElement {
// TODO:(wg-performance)(#28781) Ensure owners_.scheduleUnlayout() is
// called for diff elements that are removed
this.diff_(container, elements);

// For diffable amp-list, we have to wait until DOM diffing is done here to scan for new bindings
// (vs. asynchronously in updateBindings()), and scan the entire container (vs. just `elements`).
//
// This is because instead of replacing the entire DOM subtree, the diffing process removes
// select children from `elements` and inserts them into the container. This results in a race
// between diff_() and Bind.rescan(), which we avoid by delaying the latter until now.
this.scanForBindings_([container], [container]);
} else {
if (!opt_append) {
this.owners_./*OK*/ scheduleUnlayout(this.element, container);
Expand Down
44 changes: 0 additions & 44 deletions extensions/amp-list/0.1/test/test-amp-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -1413,50 +1413,6 @@ describes.repeated(
}
);
});

describe('rescan vs. diff race', () => {
async function rescanVsDiffTest() {
env.sandbox.spy(list, 'diff_');
env.sandbox.spy(list, 'render_');

// Diffing is skipped if there's no existing children to diff against.
const oldChild = doc.createElement('p');
oldChild.textContent = 'foo';
list.container_.appendChild(oldChild);

const newChild = doc.createElement('p');
newChild.textContent = 'bar';
// New children must have at least one binding to trigger rescan.
newChild.setAttribute('i-amphtml-binding', '');
const rendered = expectFetchAndRender(DEFAULT_FETCHED_DATA, [
newChild,
]);
await list.layoutCallback().then(() => rendered);
}

it('without diffing, should rescan _before_ render', async () => {
await rescanVsDiffTest();

expect(list.diff_).to.not.have.been.called;
expect(bind.rescan).to.have.been.calledOnce;

// Without diffable, rescan should happen before rendering the new children.
expect(bind.rescan).calledBefore(list.render_);
});

it('with diffing, should rescan _after_ render/diff', async () => {
element.setAttribute('diffable', '');

await rescanVsDiffTest();

expect(list.diff_).to.have.been.calledOnce;
expect(bind.rescan).to.have.been.calledOnce;

// With diffable, rescanning must happen after rendering (diffing) the new children.
expect(bind.rescan).calledAfter(list.render_);
expect(bind.rescan).calledAfter(list.diff_);
});
});
});

describe('binding="no"', () => {
Expand Down

0 comments on commit 497fe1d

Please sign in to comment.