amp-bind: Warn during verify instead of error (ampproject#13236)

* change amp-bind verification to user warning and clarify message * fix presubmit * fix test
protonate · Mar 15, 2018 · a071898 · a071898
1 parent dc2ac3e
commit a071898
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 14 deletions.
diff --git a/extensions/amp-bind/0.1/bind-impl.js b/extensions/amp-bind/0.1/bind-impl.js
@@ -962,11 +962,12 @@ export class Bind {
    * @private
    */
   verifyBinding_(boundProperty, element, expectedValue) {
-    const property = boundProperty.property;
+    const {property, expressionString} = boundProperty;
+    const tagName = element.tagName;
 
     // Don't show a warning for bind-only attributes,
     // like 'slide' on amp-carousel.
-    const bindOnlyAttrs = BIND_ONLY_ATTRIBUTES[element.tagName];
+    const bindOnlyAttrs = BIND_ONLY_ATTRIBUTES[tagName];
     if (bindOnlyAttrs && bindOnlyAttrs.includes(property)) {
       return;
     }
@@ -1023,11 +1024,11 @@ export class Bind {
     }
 
     if (!match) {
-      const err = user().createError(`${TAG}: ` +
-        `Default value for [${property}] does not match first expression ` +
-        `result (${expectedValue}). This can result in unexpected behavior ` +
-        'after the next state change.');
-      reportError(err, element);
+      user().warn(TAG,
+          `Default value for <${tagName} [${property}]="${expressionString}"> `
+          + `does not match first result (${expectedValue}). We recommend `
+          + 'writing expressions with matching default values, but this can be '
+          + 'safely ignored if intentional.');
     }
   }
 

diff --git a/extensions/amp-bind/0.1/test/integration/test-bind-impl.js b/extensions/amp-bind/0.1/test/integration/test-bind-impl.js
@@ -287,20 +287,21 @@ describe.configure().ifNewChrome().run('Bind', function() {
       createElement(env, container, '[class]="\'foo\'" class=" foo "');
       createElement(env, container, '[class]="\'\'"');
       createElement(env, container, '[class]="\'bar\'" class="qux"'); // Error
-      const errorSpy = env.sandbox.spy(user(), 'createError');
+      const warnSpy = env.sandbox.spy(user(), 'warn');
       return onBindReady(env, bind).then(() => {
-        expect(errorSpy).to.be.calledOnce;
-        expect(errorSpy).calledWithMatch(/bar/);
+        expect(warnSpy).to.be.calledOnce;
+        expect(warnSpy).calledWithMatch('amp-bind', /\[class\]/);
       });
     });
 
     it('should verify string attribute bindings in dev mode', () => {
       window.AMP_MODE = {development: true, test: true};
       // Only the initial value for [a] binding does not match.
       createElement(env, container, '[text]="\'a\'" [class]="\'b\'" class="b"');
-      const errorSpy = env.sandbox.spy(user(), 'createError');
+      const warnSpy = env.sandbox.spy(user(), 'warn');
       return onBindReady(env, bind).then(() => {
-        expect(errorSpy).to.be.calledOnce;
+        expect(warnSpy).to.be.calledOnce;
+        expect(warnSpy).calledWithMatch('amp-bind', /\[text\]/);
       });
     });
 
@@ -309,9 +310,10 @@ describe.configure().ifNewChrome().run('Bind', function() {
       createElement(env, container, '[disabled]="true" disabled', 'button');
       createElement(env, container, '[disabled]="false"', 'button');
       createElement(env, container, '[disabled]="true"', 'button'); // Mismatch.
-      const errorSpy = env.sandbox.spy(user(), 'createError');
+      const warnSpy = env.sandbox.spy(user(), 'warn');
       return onBindReady(env, bind).then(() => {
-        expect(errorSpy).to.be.calledOnce;
+        expect(warnSpy).to.be.calledOnce;
+        expect(warnSpy).calledWithMatch('amp-bind', /\[disabled\]/);
       });
     });