better invalid unit error handling

src/diagram/custom-math-js.test.ts

@@ -1,9 +1,18 @@
 import { createMath } from "./custom-mathjs";
 import { UnitsManager } from "./units-manager";
+import { IMathLib } from "./utils/mathjs-utils";
 
 describe("MathJS", () => {
-  const unitsManager = new UnitsManager();
-  const { evaluate, unit } = createMath(unitsManager);
+  let unitsManager: UnitsManager;
+  let evaluate: IMathLib["evaluate"];
+  let unit: IMathLib["unit"];
+
+  beforeEach(() => {
+    unitsManager = new UnitsManager();
+    const math = createMath(unitsManager);
+    evaluate = math.evaluate;
+    unit = math.unit;
+  });
 
   it("can handle unit conversion with evaluate and unit values", () => {
     const scope = {
@@ -213,6 +222,40 @@ describe("MathJS", () => {
       expect(simpl.toString()).toEqual("1 m");
     });
 
+    it("handles '_'", () => {
+      unitsManager.addUnit("t_s");
+      const localMath = createMath(unitsManager);
+      const scope = {
+        a: localMath.unit(1, "m/t_s"),
+        b: localMath.unit(1, "t_s")
+      };
+      const result = localMath.evaluate("a*b", scope);
+      const simpl = result.simplify();
+      expect(simpl.toString()).toEqual("1 m");
+    });
+
+    // This section describes the valid variable characters:
+    // https://mathjs.org/docs/expressions/syntax.html#constants-and-variables
+    it("handles a valid variable which is an invalid unit", () => {
+      unitsManager.addUnit("Ā");
+      const localMath = createMath(unitsManager);
+      const scope = {
+        a: localMath.unit(1, "m")
+      };
+      const result = localMath.evaluate("a", scope);
+      expect(result.toString()).toEqual("1 m");
+    });
+
+    it("handles an invalid variable", () => {
+      unitsManager.addUnit("✔");
+      const localMath = createMath(unitsManager);
+      const scope = {
+        a: localMath.unit(1, "m")
+      };
+      const result = localMath.evaluate("a", scope);
+      expect(result.toString()).toEqual("1 m");
+    });
+
     it("handles units with options", () => {
       unitsManager.addUnit("cat", { aliases: ["cats"]});
       const localMath = createMath(unitsManager);

src/diagram/models/variable.test.ts

@@ -338,27 +338,12 @@ describe("Variable", () => {
         {id: "variable", inputs: ["input"], expression: "a to cm/things"}
       ]
     });
-    container.unitsManager.addUnit("thing", { aliases: ["things"] });
     const variable = container.items[1] as VariableType;
 
     expect(variable.computedValueIncludingMessageAndError).toEqual({value: 900});
     expect(variable.computedUnitIncludingMessageAndError).toEqual({unit: "cm / things"});
   });
 
-  it("with a invalid unit in A it returns without crashing", () => {
-    const container = VariablesContainer.create({
-      items: [
-        {id: "input", name: "a", value: 9, unit: "m/"},
-        {id: "variable", inputs: ["input"], expression: "a to cm"}
-      ]
-    });
-    const variable = container.items[1] as VariableType;
-
-    expect(variable.computedValueIncludingMessageAndError.message?.short).toEqual("Warning: cannot compute value from inputs");
-    expect(variable.computedUnitIncludingMessageAndError.error?.short).toEqual("Warning: invalid input units");
-
-  });
-
   it("with 2 inputs with units and operation multiply it returns result", () => {
     const container = VariablesContainer.create({
       items: [
@@ -644,20 +629,71 @@ describe("Variable", () => {
     expect(variable.computedValueIncludingMessageAndError).toEqual({value: 0.01});
   });
 
-  it("handles invalid units", () => {
-    // This can happen when a user is typing a unit
-    const container = VariablesContainer.create({
-      items: [
-        {id: "inputA", name: "a", value: 1, unit: "m/"},
-        {id: "inputB", name: "b", value: 100, unit: "s"},
-        {id: "variable", inputs: ["inputA", "inputB"],
-          expression: "a*b"}
-      ]
-    });
-    const variable = container.items[2] as VariableType;
-
-    expect(variable.computedUnitIncludingMessageAndError.error?.short).toEqual("Warning: invalid input units");
-    expect(variable.computedValueIncludingMessageAndError.message?.short).toEqual("Warning: cannot compute value from inputs");
+  describe("invalid units", () => {
+    it("handles incomplete unit strings", () => {
+      // This can happen when a user is typing a unit
+      const container = VariablesContainer.create({
+        items: [
+          {id: "inputA", name: "a", value: 1, unit: "m/"},
+          {id: "inputB", name: "b", value: 100, unit: "s"},
+          {id: "variable", inputs: ["inputA", "inputB"],
+            expression: "a*b"}
+        ]
+      });
+      const variable = container.items[2] as VariableType;
+
+      expect(variable.computedUnitIncludingMessageAndError.error?.short).toEqual("Warning: invalid input units");
+      expect(variable.computedValueIncludingMessageAndError.message?.short).toEqual("Warning: cannot compute value from inputs");
+    });  
+
+    it("handles incomplete unit conversion without crashing", () => {
+      const container = VariablesContainer.create({
+        items: [
+          {id: "input", name: "a", value: 9, unit: "m/"},
+          {id: "variable", inputs: ["input"], expression: "a to cm"}
+        ]
+      });
+      const variable = container.items[1] as VariableType;
+
+      expect(variable.computedValueIncludingMessageAndError.message?.short).toEqual("Warning: cannot compute value from inputs");
+      expect(variable.computedUnitIncludingMessageAndError.error?.short).toEqual("Warning: invalid input units");
+
+    });
+
+    // Currently we only allow units to be letters, `$`, and `_`
+    // Numbers are allowed if they aren't the first character
+    // A string which is not a valid "variable" will go through one code path.
+    // A string which is a valid "variable" will go through a different code path.
+    // The char `Ā` is a valid variable, but not a valid unit.
+    // This section describes the valid variable characters:
+    // https://mathjs.org/docs/expressions/syntax.html#constants-and-variables
+    it("handles valid variable which is not a valid unit", () => {
+      const container = VariablesContainer.create({
+        items: [
+          {id: "inputA", name: "a", value: 1, unit: "Ā"},
+          {id: "variable", inputs: ["inputA"],
+            expression: "a"}
+        ]
+      });
+      const variable = container.items[1] as VariableType;
+
+      expect(variable.computedUnitIncludingMessageAndError.error?.short).toEqual("Warning: invalid input units");
+      expect(variable.computedValueIncludingMessageAndError.message?.short).toEqual("Warning: cannot compute value from inputs");
+    });  
+
+    it("handles invalid variable character", () => {
+      const container = VariablesContainer.create({
+        items: [
+          {id: "inputA", name: "a", value: 1, unit: "✔"},
+          {id: "variable", inputs: ["inputA"],
+            expression: "a"}
+        ]
+      });
+      const variable = container.items[1] as VariableType;
+
+      expect(variable.computedUnitIncludingMessageAndError.error?.short).toEqual("Warning: invalid input units");
+      expect(variable.computedValueIncludingMessageAndError.message?.short).toEqual("Warning: cannot compute value from inputs");
+    });  
   });
 
   it("handles adding compatible inputs first with a value and second without a value", () => {

src/diagram/units-manager.ts

@@ -32,7 +32,7 @@ export class UnitsManager {
 
     const isAlphaOriginal = mathUnit.isValidAlpha;
     mathUnit.isValidAlpha = function (c: string): boolean {
-      return isAlphaOriginal(c) || c === "$";
+      return isAlphaOriginal(c) || c === "$" || c === "_";
     };
 
     deleteUnits.forEach((u: string) => mathUnit.deleteUnit(u));
@@ -42,10 +42,16 @@ export class UnitsManager {
       //   createUnit(units: Record<string, string | UnitDefinition>, options?: CreateUnitOptions): Unit;
       // If u.options is undefined and we call `m.createUnit(u.unit, u.options);` Typescript's overloading
       // support fails for some reason. So this is split out to 2 function calls.
-      if (u.options) {
-        m.createUnit(u.unit, u.options);
-      } else {
-        m.createUnit(u.unit);
+      try {
+        if (u.options) {
+          m.createUnit(u.unit, u.options);
+        } else {
+          m.createUnit(u.unit);
+        }  
+      } catch (e) {
+        // Ignore any invalid units. If we remove them from the units array that will trigger loop.
+        // In practice no invalid units should be added because they should be skipped during the call 
+        // to `getMathUnit`.
       }
     });
 
@@ -91,6 +97,9 @@ export class UnitsManager {
     try {
       const isValidUnknownUnitName = (name: string) => {
         // MathJS doesn't handle empty string units.        
+        // NOTE: this isn't really checking if the string is valid, it is just making sure it isn't
+        // empty and is unknown. The Unit library is more strict about what is a valid unit,
+        // however there isn't an easy way to access this without just trying to create a Value
         return name && !mathLib.Unit.isValuelessUnit(name);
       };
 
@@ -99,7 +108,6 @@ export class UnitsManager {
       };
 
       const registerUnit = (name: string, options?: CreateUnitOptions) => {
-        this.addUnit(name, options);
         // NOTE: this `createUnit` call only adds the unit to the passed in mathLib. 
         // Each variable has its own mathLib, so the other variable's mathLibs are not
         // updated here.
@@ -111,6 +119,11 @@ export class UnitsManager {
           // mathLib.createUnit fails if options is undefined
           mathLib.createUnit(name);
         }
+
+        // This is called after the createUnit call on purpose. The createUnit call above will
+        // throw an exception if the unit is invalid. In that case this addUnit call will never
+        // happen. So the invalid unit will not be added to the unit manager.
+        this.addUnit(name, options);
       };
 
       // Look for unknown units in the unit string