From f41e3becf56d99f3a7b5f4bbc0906685f72a6766 Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Thu, 11 Mar 2021 23:25:09 -0500 Subject: [PATCH] Revert "amp-list: Fix Bind.rescan vs. diffing race condition (#32650)" This reverts commit a0f71dfb0381e236d6c6be287549748838731467. --- extensions/amp-list/0.1/amp-list.js | 58 +++++-------------- extensions/amp-list/0.1/test/test-amp-list.js | 44 -------------- 2 files changed, 13 insertions(+), 89 deletions(-) diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js index 3e0619c50dc7..619d8d14b182 100644 --- a/extensions/amp-list/0.1/amp-list.js +++ b/extensions/amp-list/0.1/amp-list.js @@ -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); } } @@ -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} addedElements - * @param {!Array} 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} elements * @param {boolean=} opt_append @@ -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); diff --git a/extensions/amp-list/0.1/test/test-amp-list.js b/extensions/amp-list/0.1/test/test-amp-list.js index 063e6393e98a..b725dcf2e33f 100644 --- a/extensions/amp-list/0.1/test/test-amp-list.js +++ b/extensions/amp-list/0.1/test/test-amp-list.js @@ -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"', () => {