Throw halting errors during analysis.

Eventually, we want analysis to be one-time, static work that is cached per class. Therefore, we should not rely on having a target on which to dispatch errors. Closes #31
Netflix · May 11, 2019 · e8b343e · e8b343e
1 parent 135a366
commit e8b343e
Show file tree

Hide file tree

Showing 5 changed files with 141 additions and 118 deletions.
diff --git a/mixins/property-effects-mixin.js b/mixins/property-effects-mixin.js
@@ -25,7 +25,7 @@ export default superclass =>
           return { methodName: candidate.name, dependencies };
         }
       } catch (err) {
-        // Bad input. Ignore.
+        throw new Error(`Malformed computed "${computed}".`);
       }
     }
 
@@ -36,8 +36,7 @@ export default superclass =>
       } else if (target.constructor[methodName] instanceof Function) {
         return target.constructor[methodName].bind(target.constructor);
       } else {
-        const err = new Error(`Cannot resolve methodName "${methodName}".`);
-        target.dispatchError(err);
+        throw new Error(`Cannot resolve methodName "${methodName}".`);
       }
     }
 
@@ -100,35 +99,28 @@ export default superclass =>
         if (definition.computed) {
           hasComputedProperties = true;
           const { computed } = definition;
-          const parsedComputed = this.parseComputed(computed);
-          if (parsedComputed) {
-            const { methodName, dependencies } = parsedComputed;
-            for (const dependency of dependencies) {
-              if (dependency in properties === false) {
-                const err = new Error(`Missing dependency "${dependency}".`);
-                target.dispatchError(err);
-              }
+          const { methodName, dependencies } = this.parseComputed(computed);
+          for (const dependency of dependencies) {
+            if (dependency in properties === false) {
+              throw new Error(`Missing dependency "${dependency}".`);
             }
-            const callback = this.createComputedCallback(
-              target,
-              property,
-              methodName,
-              dependencies
-            );
-            if (callback) {
-              dependentToCallback[property] = callback;
+          }
+          const callback = this.createComputedCallback(
+            target,
+            property,
+            methodName,
+            dependencies
+          );
+          if (callback) {
+            dependentToCallback[property] = callback;
+          }
+          for (const dependency of dependencies) {
+            if (dependency in dependencyToDependents === false) {
+              dependencyToDependents[dependency] = [];
             }
-            for (const dependency of dependencies) {
-              if (dependency in dependencyToDependents === false) {
-                dependencyToDependents[dependency] = [];
-              }
-              if (property in dependencyToDependents[dependency] === false) {
-                dependencyToDependents[dependency].push(property);
-              }
+            if (property in dependencyToDependents[dependency] === false) {
+              dependencyToDependents[dependency].push(property);
             }
-          } else {
-            const err = new Error(`Malformed computed "${computed}".`);
-            target.dispatchError(err);
           }
         }
       }
@@ -159,7 +151,7 @@ export default superclass =>
               callbacks.forEach(callback => callback());
           }
         } else {
-          target.dispatchError(new Error('Computed properties are cyclic.'));
+          throw new Error('Computed properties are cyclic.');
         }
       }
     }

diff --git a/test/fixture-element-computed-properties.js b/test/fixture-element-computed-properties.js
@@ -109,42 +109,57 @@ customElements.define(
   TestElementComputedProperties
 );
 
-class TestElementComputedPropertiesErrors extends XElementProperties {
+class TestElementComputedPropertiesErrorsMalformed extends XElementProperties {
+  static get properties() {
+    return { malformed: { type: Boolean, computed: 'malformed(a,,b)' } };
+  }
+}
+customElements.define(
+  'test-element-computed-properties-errors-malformed',
+  TestElementComputedPropertiesErrorsMalformed
+);
+
+class TestElementComputedPropertiesErrorsUnresolved extends XElementProperties {
   static get properties() {
     return {
-      malformed: {
-        type: Boolean,
-        computed: 'malformed(a,,b)',
-      },
-      dne: {
-        type: Boolean,
-        computed: 'thisDNE(malformed)',
-      },
-      missing: {
-        type: String,
-        computed: 'computeMissing(notDeclared)',
-      },
-      zz: {
-        type: Boolean,
-      },
-      cyclic: {
-        type: String,
-        computed: 'computeCyclic(zz, cyclic)',
-      },
+      a: { type: String },
+      dne: { type: Boolean, computed: 'thisDNE(a)' },
     };
   }
-  static computeMissing(notDeclared) {
-    return `this is just here to get past the unresolved method check`;
-  }
-  static computeCyclic(zz, cyclic) {
-    return `this is just here to get past the unresolved method check`;
-  }
-  static template() {
-    return () => ``;
+}
+customElements.define(
+  'test-element-computed-properties-errors-unresolved',
+  TestElementComputedPropertiesErrorsUnresolved
+);
+
+class TestElementComputedPropertiesErrorsMissing extends XElementProperties {
+  static get properties() {
+    return { missing: { type: String, computed: 'computeMissing(notHere)' } };
   }
+  static computeMissing() {}
 }
+customElements.define(
+  'test-element-computed-properties-errors-missing',
+  TestElementComputedPropertiesErrorsMissing
+);
 
+class TestElementComputedPropertiesErrorsCyclic extends XElementProperties {
+  static get properties() {
+    return {
+      a: { type: Boolean },
+      cyclic: { type: Boolean, computed: 'computeCyclic(a, cyclic)' },
+    };
+  }
+  static computeCyclic() {}
+}
 customElements.define(
-  'test-element-computed-properties-errors',
-  TestElementComputedPropertiesErrors
+  'test-element-computed-properties-errors-cyclic',
+  TestElementComputedPropertiesErrorsCyclic
 );
+
+export {
+  TestElementComputedPropertiesErrorsMalformed,
+  TestElementComputedPropertiesErrorsUnresolved,
+  TestElementComputedPropertiesErrorsMissing,
+  TestElementComputedPropertiesErrorsCyclic,
+};
diff --git a/test/fixture-element-observed-properties.js b/test/fixture-element-observed-properties.js
@@ -24,10 +24,6 @@ class TestElementObservedProperties extends XElementProperties {
         reflect: true,
         observer: 'observePopped',
       },
-      dne: {
-        type: Boolean,
-        observer: 'thisDNE',
-      },
     };
   }
   computeC(a, b) {
@@ -90,3 +86,16 @@ customElements.define(
   'test-element-observed-properties',
   TestElementObservedProperties
 );
+
+
+class TestElementObservedPropertiesErrorsUnresolved extends XElementProperties {
+  static get properties() {
+    return { dne: { type: Boolean, observer: 'thisDNE' } };
+  }
+}
+customElements.define(
+  'test-element-observed-properties-errors-unresolved',
+  TestElementObservedPropertiesErrorsUnresolved
+);
+
+export { TestElementObservedPropertiesErrorsUnresolved };
diff --git a/test/test-computed-properties.js b/test/test-computed-properties.js
@@ -1,5 +1,10 @@
 import { suite, it } from './runner.js';
-import './fixture-element-computed-properties.js';
+import {
+  TestElementComputedPropertiesErrorsMalformed,
+  TestElementComputedPropertiesErrorsUnresolved,
+  TestElementComputedPropertiesErrorsMissing,
+  TestElementComputedPropertiesErrorsCyclic,
+} from './fixture-element-computed-properties.js';
 
 const parsingTestCases = [
   {
@@ -35,7 +40,7 @@ const parsingTestCases = [
         b,
       )
     `,
-    expected: undefined,
+    error: true,
   },
   {
     label: 'does not allow spaces in tokens',
@@ -45,80 +50,83 @@ const parsingTestCases = [
         b,
       )
     `,
-    expected: undefined,
+    error: true,
   },
   {
     label: 'does not allow commas in method name',
     computed: 'comp,uteC(a, b)',
-    expected: undefined,
+    error: true,
   },
   {
     label: 'does not allow spaces in method name',
     computed: 'comp uteC(a, b)',
-    expected: undefined,
+    error: true,
   },
   {
     label: 'does not allow parentheses in tokens (0)',
     computed: 'computeC(a), b)',
-    expected: undefined,
+    error: true,
   },
   {
     label: 'does not allow parentheses in tokens (1)',
     computed: 'computeC(a(, b)',
-    expected: undefined,
+    error: true,
   },
 ];
 
 suite('x-element computed properties', ctx => {
-  // Test errors
-  const malformedMessage = `Malformed computed "malformed(a,,b)".`;
-  const unresolvedMessage = `Cannot resolve methodName "thisDNE".`;
-  const missingMessage = `Missing dependency "notDeclared".`;
-  const cyclicMessage = 'Computed properties are cyclic.';
-
+  // Test analysis-time errors.
   let malformed = false;
-  let unresolved = false;
-  let missing = false;
-  let cyclic = false;
-
-  const onErrorTest = evt => {
-    if (evt.error.message === malformedMessage) {
-      malformed = true;
-    } else if (evt.error.message === unresolvedMessage) {
-      unresolved = true;
-    } else if (evt.error.message === missingMessage) {
-      missing = true;
-    } else if (evt.error.message === cyclicMessage) {
-      cyclic = true;
-    } else {
-      console.error(evt.error);
-    }
-  };
-  document.addEventListener('error', onErrorTest);
-
-  ctx.body.appendChild(
-    document.createElement('test-element-computed-properties-errors')
-  );
-
-  it('should error for malformed computed DSL', malformed);
+  try {
+    new TestElementComputedPropertiesErrorsMalformed();
+  } catch (err) {
+    malformed = err.message === `Malformed computed "malformed(a,,b)".`;
+  }
+  it('should throw halting error for malformed computed DSL', malformed);
 
-  it('should error for unresolved method names', unresolved);
+  let unresolved = false;
+  try {
+    new TestElementComputedPropertiesErrorsUnresolved();
+  } catch (err) {
+    unresolved = err.message === `Cannot resolve methodName "thisDNE".`;
+  }
+  it('should throw halting error for unresolved method names', unresolved);
 
-  it('should error for missing dependencies', missing);
+  let missing = false;
+  try {
+    new TestElementComputedPropertiesErrorsMissing();
+  } catch (err) {
+    missing = err.message === `Missing dependency "notHere".`;
+  }
+  it('should throw halting error for missing dependencies', missing);
 
+  let cyclic = false;
+  try {
+    new TestElementComputedPropertiesErrorsCyclic();
+  } catch (err) {
+    cyclic = err.message === 'Computed properties are cyclic.';
+  }
   it('should error for cyclic dependency graphs', cyclic);
 
-  document.removeEventListener('error', onErrorTest);
-
   // Test normal use case.
   document.addEventListener('error', evt => console.error(evt.error));
   const el = document.createElement('test-element-computed-properties');
   ctx.body.appendChild(el);
 
   // Test parsing of computed DSL.
-  for (const { label, computed, expected } of parsingTestCases) {
-    const actual = el.constructor.parseComputed(computed);
-    it(label, JSON.stringify(actual) === JSON.stringify(expected));
+  for (const { label, computed, expected, error } of parsingTestCases) {
+    if (error) {
+      let errored = false;
+      try {
+        el.constructor.parseComputed(computed);
+      } catch (err) {
+        errored = err.message === `Malformed computed "${computed}".`;
+      }
+      it(label, errored);
+    } else {
+      const actual = el.constructor.parseComputed(computed);
+      it(label, JSON.stringify(actual) === JSON.stringify(expected));
+    }
   }
 
   it(

diff --git a/test/test-observed-properties.js b/test/test-observed-properties.js
@@ -1,5 +1,5 @@
 import { suite, it } from './runner.js';
-import './fixture-element-observed-properties.js';
+import { TestElementObservedPropertiesErrorsUnresolved } from './fixture-element-observed-properties.js';
 
 const isObject = obj => obj instanceof Object && obj !== null;
 const deepEqual = (a, b) => {
@@ -16,16 +16,17 @@ const deepEqual = (a, b) => {
 };
 
 suite('x-element observed properties', ctx => {
-  const unresolvedMessage = `Cannot resolve methodName "thisDNE".`;
-
+  // Test analysis-time errors.
   let unresolved = false;
+  try {
+    new TestElementObservedPropertiesErrorsUnresolved();
+  } catch (err) {
+    unresolved = err.message === `Cannot resolve methodName "thisDNE".`;
+  }
+  it('should error for unresolved method names', unresolved);
 
   document.onerror = evt => {
-    if (evt.error.message === unresolvedMessage) {
-      unresolved = true;
-    } else {
-      console.error(evt.error);
-    }
+    console.error(evt.error);
   };
 
   const el = document.createElement('test-element-observed-properties');
@@ -34,8 +35,6 @@ suite('x-element observed properties', ctx => {
 
   ctx.body.appendChild(el);
 
-  it('should error for unresolved method names', unresolved);
-
   it(
     'initialized as expected',
     deepEqual(el.changes, [