Add the ability to add and remove bindings in subtrees rooted at a specified node by kmh287 · Pull Request #7439 · ampproject/amphtml

kmh287 · 2017-02-08T23:40:59Z

Adds the ability to add and remove bindings in subtrees rooted at a specific node
Adds tests for the new behavior

…cific node

dreamofabear · 2017-02-09T19:10:03Z

extensions/amp-bind/0.1/bind-impl.js

-      this.evaluator_ = new BindEvaluator();
-      const parseErrors = this.evaluator_.setBindings(bindings);
+      this.evaluator_ = this.evaluator_ || new BindEvaluator();
+      const parseErrors = this.evaluator_.setBindings(this.bindings_);


This will cause the evaluator to re-parse every expression, and expression parsing currently is the slowest operation in amp-bind. Instead, we should support incrementally add of expressions.

You can probably avoid the this.bindings_ variable, which saves work when removing bindings from a subtree.

dreamofabear · 2017-02-09T19:12:18Z

extensions/amp-bind/0.1/bind-impl.js

+              }
+            }
+            if (elements.length == 0) {
+              delete this.expressionToElements_[expression];


We should also remove obsolete expressions from the evaluator.

dreamofabear · 2017-02-09T19:15:17Z

extensions/amp-bind/0.1/bind-impl.js

+   * @return {Promise}
+   */
+  removeBindingsForSubtree(rootElement) {
+    this.scanPromise_ = this.scanSubtree_(rootElement).then(results => {


I don't think we need to scan the subtree here since we should already have the bindings/expression strings that we're about to remove.

dreamofabear · 2017-02-09T19:18:06Z

extensions/amp-bind/0.1/bind-impl.js

+          const currentElement = this.boundElements_[j];
+          if (currentElement
+                .element
+                .isEqualNode(boundElementToRemove.element)) {


We want to match the exact node, not just equivalent nodes. Otherwise, we may remove bindings from elements outside of the subtree.

Will exactly the same node always be represented by the same object? i.e. will === work?

https://developer.mozilla.org/en-US/docs/Web/API/Node/isSameNode

dreamofabear · 2017-02-09T19:19:16Z

extensions/amp-bind/0.1/bind-impl.js

+    this.scanPromise_ = this.scanSubtree_(rootElement).then(results => {
+      const {boundElements, bindings, expressionToElements} = results;
+
+      // TODO(kmh287): Discuss strategies for speedup


You can probably just walk the ancestor list of each bound element and check whether rootElement is one of them.

dreamofabear · 2017-02-09T19:20:52Z

extensions/amp-bind/0.1/bind-impl.js

+   * @param {!Element} rootElement
+   * @return {Promise}
+   */
+  addBindingsForSubtree(rootElement) {


Nit: s/Subtree/Node and s/rootElement/node

dreamofabear · 2017-02-09T19:22:38Z

extensions/amp-bind/0.1/bind-impl.js

+   * @param {!Element} rootElement
+   * @return {Promise}
+   */
+  addBindingsForSubtree(rootElement) {


Mark all private methods with @private attribute and trailing underscore. This allows Closure compiler to rename the method during minimization and other optimizations like DCE.

dreamofabear · 2017-02-09T19:23:09Z

extensions/amp-bind/0.1/test/test-bind-impl.js

   * @return {!Promise}
   */
-  function onBindReady(callback) {
+  function onBindReady() {


dreamofabear · 2017-02-09T19:23:29Z

extensions/amp-bind/0.1/test/test-bind-impl.js

  });

-  it('should scan for bindings when ampdoc is ready', () => {
+  it('should scan for bindings when ampdoc is ready', done => {


Remove unused done here and below.

Now that onBindReady() returns a promise, it looks like we need this to signal the tests are done after the promises resolve.

Shouldn't be necessary. Grab me if it's not working.

Mocha understand promise return values, so there's no need for done.

dreamofabear · 2017-02-09T19:24:33Z

extensions/amp-bind/0.1/test/test-bind-impl.js

   * @return {!Element}
   */
  function createElementWithBinding(binding) {
+    const fakeDiv = env.win.document.createElement('div');


Why this change? Also, is this div actually "fake"?

The old version only ever created one element with a binding. I needed to change it to make one per call.

…e_binding

jridgewell · 2017-02-13T22:37:13Z

extensions/amp-bind/0.1/test/test-bind-impl.js

  });

-  it('should scan for bindings when ampdoc is ready', () => {
+  it('should scan for bindings when ampdoc is ready', done => {


Mocha understand promise return values, so there's no need for done.

jridgewell · 2017-02-13T22:43:27Z

extensions/amp-bind/0.1/bind-impl.js

+   * @param {!Element} potentialParent
+   * @return {boolean}
+   */
+  isAncestorOf_(node, potentialParent) {


Nodes have a super efficient #contains to do this:

isAncestorOf_(node, potentialParent) { return potentialParent.contains(node); }

Awesome. Done.

This is no longer called, right? Please delete.

jridgewell · 2017-02-13T22:45:20Z

extensions/amp-bind/0.1/bind-impl.js

+  removeBindingsForNode_(node) {
+    return new Promise(resolve => {
+      // Eliminate bound elements that have node as an ancestor
+      for (let i = this.boundElements_.length - 1; i >= 0; i--) {


We have a filterSplice helper for this.

jridgewell · 2017-02-14T00:41:41Z

extensions/amp-bind/0.1/bind-evaluator.js

+    for (let i = 0; i < expressionStrings.length; i++) {
+      expressionsToRemove[expressionStrings[i]] = undefined;
+    }
+


Ditto filterSplice

jridgewell · 2017-02-14T00:41:53Z

extensions/amp-bind/0.1/bind-impl.js

+      const deletedExpressions = [];
+      for (const expression in this.expressionToElements_) {
+        const elements = this.expressionToElements_[expression];
+        for (let j = elements.length - 1; j >= 0; j--) {


jridgewell · 2017-02-14T00:42:20Z

extensions/amp-bind/0.1/bind-impl.js

+   * @param {!Element} potentialParent
+   * @return {boolean}
+   */
+  isAncestorOf_(node, potentialParent) {


This is no longer called, right? Please delete.

kmh287 · 2017-02-14T16:21:20Z

@choumx I've merged in the web worker. PTAL

dreamofabear

Looks good, mostly nits.

dreamofabear · 2017-02-14T15:38:49Z

extensions/amp-bind/0.1/bind-impl.js

+   */
+  removeBindingsForNode_(node) {
+    return new Promise(resolve => {
+


Nit: Extra line.

dreamofabear · 2017-02-14T15:39:35Z

extensions/amp-bind/0.1/bind-impl.js

+  removeBindingsForNode_(node) {
+    return new Promise(resolve => {
+
+      // Eliminate bound elements that have node as an ancestor


Nit: Periods at end of comments to be consistent.

dreamofabear · 2017-02-14T15:45:57Z

extensions/amp-bind/0.1/bind-evaluator.js

+      expressionsToRemove[expressionStrings[i]] = undefined;
+    }
+
+    this.parsedBindings_ = filterSplice(this.parsedBindings_, binding => {


Nit: "expressionString" or "string" to disambiguate from BindExpression object.

dreamofabear · 2017-02-14T15:46:58Z

extensions/amp-bind/0.1/bind-evaluator.js

+    }
+
+    this.parsedBindings_ = filterSplice(this.parsedBindings_, binding => {
+      const expression = binding.expression.expressionString;


hasOwnProperty not necessary for pure map objects created by Object.create(null). Set map values to true above and check expressionsToRemove[expressionString].

dreamofabear · 2017-02-14T15:49:42Z

extensions/amp-bind/0.1/test/test-bind-evaluator.js

+  });
+
+  it('should allow callers to add bindings multiple times', () => {
+    const bindingDef = {


Nit: Drop "Def", which is only needed as a naming convention for @typedef. Though since you only reference each var once here, inlining would be most readable.

https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-best-practices.shtml?cl=head#constants

dreamofabear · 2017-02-14T16:04:12Z

extensions/amp-bind/0.1/bind-impl.js

+        }
+      }
+
+      // Remove the bindings from the evaluator


dreamofabear · 2017-02-14T16:05:10Z

extensions/amp-bind/0.1/bind-evaluator.js

+  /**
+   * Removes all parsed bindings for the provided expressions.
+   * @param {!Array<string>} expressionStrings
+   */


Nit: removeBindingsWithExpressionStrings.

dreamofabear · 2017-02-14T16:05:31Z

extensions/amp-bind/0.1/test/test-bind-evaluator.js

+
+import {BindEvaluator, BindingDef} from '../bind-evaluator';
+
+describe('BindEvaluator', () => {


Thanks for adding tests to this class!

dreamofabear · 2017-02-14T16:06:23Z

extensions/amp-bind/0.1/test/test-bind-impl.js

+    return onBindReady().then(() => {
+      expect(bind.boundElements_.length).to.equal(5);
+      return bind
+        .removeBindingsForNode_(env.win.document.getElementById('parent'));


Nit: Move env.win.document to a local var and this can fit on one line.

dreamofabear · 2017-02-14T16:23:22Z

extensions/amp-bind/0.1/test/test-bind-impl.js

    });
  });

+  it('should have same state after removing + re-adding a subtree', () => {


Recommend also verifying invocation of corresponding BindEvaluator methods.

IMO the method invocations on the evaluator are more of an implementation detail of the Bind object.

jridgewell · 2017-02-14T18:11:46Z

extensions/amp-bind/0.1/bind-evaluator.js

+    }
+
+    this.parsedBindings_ = filterSplice(this.parsedBindings_, binding => {
+      const expressionString = binding.expression.expressionString;


You want ! this. #filterSplice (and #filter) keep the items that return true and remove the items that return false.

Thanks for catching this.

Actually, this worked as intended because it's wrong twice. Assigning to this.parsedBindings_ assigned the elements that were filtered out.

Done.

dreamofabear · 2017-02-14T20:16:28Z

src/web-worker/web-worker.js

+      if (evaluator_) {
+        returnValue =
+          evaluator_
+          .removeBindingsWithExpressionStrings.apply(evaluator_, args);


Move evaluator_.removeBindingsWithExpressionStrings to a local var to fix this formatting.

kmh287 · 2017-02-16T13:44:14Z

Thank you for the thorough review

…ecified node (ampproject#7439) * first pass at adding/removing subtrees * tests passing * Add ability to add and remove bindings in the subtree rooted at a specific node * lint fixes * merge conflict * ci issues * some cleanup * pr comments * pr comments, new method of removing bindings for node and its children * fix some lint warnings * test and lint fixes * update comments * pr comments * lint errors * pr comments * cleanup * lint errors * pr comments * pr comments and lint warnings * typo * more lint warnings * pr comment * pr comment

kmh287 and others added 8 commits February 8, 2017 12:27

first pass at adding/removing subtrees

8d9f0af

tests passing

1f0079f

Add ability to add and remove bindings in the subtree rooted at a spe…

f7844a7

…cific node

lint fixes

bdafca6

merge

86f9f9f

merge conflict

ed83a6d

ci issues

dd7027d

some cleanup

7b8bf0b

dreamofabear self-requested a review February 9, 2017 17:52

dreamofabear requested changes Feb 9, 2017

View reviewed changes

kmh287 and others added 6 commits February 9, 2017 15:47

Merge branch 'master' of github.com:kmh287/amphtml into #7376_mustach…

fdc2f70

…e_binding

pr comments

884c091

pr comments, new method of removing bindings for node and its children

da8de76

fix some lint warnings

f775df8

test and lint fixes

764e789

update comments

1c8ef1d

kmh287 mentioned this pull request Feb 13, 2017

Weekly Status 2017-02-13 #7505

Closed

Merge branch 'master' into #7376_mustache_binding

e24d628

jridgewell reviewed Feb 13, 2017

View reviewed changes

kmh287 added 2 commits February 13, 2017 18:21

pr comments

ea642c7

lint errors

ee698b7

jridgewell reviewed Feb 14, 2017

View reviewed changes

kmh287 added 4 commits February 14, 2017 10:27

pr comments

ac0aeea

merge

3223794

cleanup

c80f2a2

lint errors

e357373

dreamofabear reviewed Feb 14, 2017

View reviewed changes

kmh287 added 2 commits February 14, 2017 12:23

pr comments

85e759e

pr comments and lint warnings

c698197

kmh287 added 2 commits February 14, 2017 12:41

typo

35d70f5

more lint warnings

cf114f6

jridgewell requested changes Feb 14, 2017

View reviewed changes

pr comment

e35a7eb

jridgewell approved these changes Feb 14, 2017

View reviewed changes

dreamofabear approved these changes Feb 14, 2017

View reviewed changes

pr comment

e9d5ad4

dreamofabear merged commit d822f3a into ampproject:master Feb 16, 2017


		import {BindEvaluator, BindingDef} from '../bind-evaluator';

		describe('BindEvaluator', () => {

Conversation

kmh287 commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmh287 commented Feb 14, 2017

Uh oh!

dreamofabear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kmh287 commented Feb 8, 2017 •

edited

Loading