Add better error messaging for failed expression eval and parsing (#239)

* Add better error messaging for failed expression eval and parsing * Remove nested-error-stacks from dep list
player-ui · Nov 16, 2023 · f5b1f77 · f5b1f77
1 parent 6ea2d4c
commit f5b1f77
Show file tree

Hide file tree

Showing 4 changed files with 16 additions and 13 deletions.
diff --git a/core/player/BUILD b/core/player/BUILD
@@ -9,8 +9,6 @@ javascript_pipeline(
         "@npm//p-defer",
         "@npm//queue-microtask",
         "@npm//tapable-ts",
-        "@npm//nested-error-stacks",
-        "@npm//@types/nested-error-stacks",
         "@npm//parsimmon",
         "@npm//@types/parsimmon",
         "@npm//arr-flatten",

diff --git a/core/player/src/expressions/__tests__/evaluator.test.ts b/core/player/src/expressions/__tests__/evaluator.test.ts
@@ -386,7 +386,7 @@ describe('evaluator', () => {
 
   test('throws errors for unknown expressions', () => {
     expect(() => evaluator.evaluate('foo()')).toThrowError(
-      'Unknown expression function: foo'
+      'Error evaluating expression: foo()'
     );
   });
 

diff --git a/core/player/src/expressions/evaluator.ts b/core/player/src/expressions/evaluator.ts
@@ -1,4 +1,5 @@
 import { SyncWaterfallHook, SyncBailHook } from 'tapable-ts';
+import { NestedError } from 'ts-nested-error';
 import { parseExpression } from './parser';
 import * as DEFAULT_EXPRESSION_HANDLERS from './evaluator-functions';
 import { isExpressionNode } from './types';
@@ -221,21 +222,27 @@ export class ExpressionEvaluator {
       [, matchedExp] = Array.from(matches); // In case the expression was surrounded by @[ ]@
     }
 
-    try {
-      const storedAST = this.expressionsCache.get(matchedExp);
+    let storedAST: ExpressionNode;
 
-      if (storedAST) {
-        return this._execAST(storedAST, options);
+    try {
+      storedAST =
+        this.expressionsCache.get(matchedExp) ?? parseExpression(matchedExp);
+      this.expressionsCache.set(matchedExp, storedAST);
+    } catch (e: any) {
+      if (options.throwErrors || !this.hooks.onError.call(e)) {
+        // Only throw the error if it's not handled by the hook, or throwErrors is true
+        throw new NestedError(`Error parsing expression: ${exp}`, e);
       }
 
-      const expAST = parseExpression(matchedExp);
-      this.expressionsCache.set(matchedExp, expAST);
+      return;
+    }
 
-      return this._execAST(expAST, options);
+    try {
+      return this._execAST(storedAST, options);
     } catch (e: any) {
       if (options.throwErrors || !this.hooks.onError.call(e)) {
         // Only throw the error if it's not handled by the hook, or throwErrors is true
-        throw e;
+        throw new NestedError(`Error evaluating expression: ${exp}`, e);
       }
     }
   }

diff --git a/package.json b/package.json
@@ -69,7 +69,6 @@
     "@types/lz-string": "^1.3.34",
     "@types/mdx-js__react": "^1.5.5",
     "@types/mkdirp": "^1.0.2",
-    "@types/nested-error-stacks": "^2.1.0",
     "@types/node": "^16.11.12",
     "@types/parsimmon": "^1.10.0",
     "@types/pubsub-js": "^1.8.3",
@@ -144,7 +143,6 @@
     "mdast-util-toc": "^6.1.0",
     "mkdirp": "^1.0.4",
     "monaco-editor": "^0.31.1",
-    "nested-error-stacks": "^2.1.1",
     "next": "^12.0.7",
     "nextjs-google-analytics": "^1.2.1",
     "p-defer": "^3.0.0",