Revert "amp-list: Fix Bind.rescan vs. diffing race condition (ampproject#32650)" (ampproject#33232)

This reverts commit a0f71df.

Author: samouri

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<!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
@@ -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);

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"', () => {