Fixed reporting of load-time errors from modules imported by specs

[#178764731]

.circleci/config.yml

@@ -10,10 +10,20 @@ executors:
     docker:
       - image: cimg/node:16.1.0
     working_directory: ~/workspace
-  node14:
+  node14_latest:
     docker:
       - image: circleci/node:14
     working_directory: ~/workspace
+  # 14.8 is the first version with top level await in ES modules.
+  node14_8:
+    docker:
+      - image: circleci/node:14.8
+    working_directory: ~/workspace
+  # 14.7 is the last version without top level await in ES modules.
+  node14_7:
+    docker:
+      - image: circleci/node:14.7
+    working_directory: ~/workspace
   node12_latest:
     docker:
     - image: circleci/node:12
@@ -22,10 +32,12 @@ executors:
     docker:
     - image: circleci/node:12.0
     working_directory: ~/workspace
+  # 12.17 is the first version with dynamic import() of commonjs modules
   node12_17:
     docker:
     - image: circleci/node:12.17
     working_directory: ~/workspace
+  # 12.17 is the last version without dynamic import() of commonjs modules
   node12_16:
     docker:
     - image: circleci/node:12.16
@@ -61,8 +73,14 @@ workflows:
           executor: node16
           name: node_16
       - build_and_test:
-          executor: node14
-          name: node_14
+          executor: node14_latest
+          name: node_14_latest
+      - build_and_test:
+          executor: node14_8
+          name: node_14_8
+      - build_and_test:
+          executor: node14_7
+          name: node_14_7
       - build_and_test:
           executor: node12_latest
           name: node_12_latest

.eslintignore

@@ -1,2 +1,3 @@
 spec/fixtures/cjs-syntax-error/syntax_error.js
+spec/fixtures/esm-importing-commonjs-syntax-error/syntax_error.js
 spec/fixtures/js-loader-import/*.js

lib/loader.js

@@ -12,16 +12,7 @@ Loader.prototype.load = function(path, alwaysImport) {
     // Node enforces this on Windows but not on other OSes.
     const url = `file://${path}`;
     return this.import_(url).catch(function(e) {
-      if (e.message.indexOf(path) !== -1 || e.stack.indexOf(path) !== -1) {
-        return Promise.reject(e);
-      } else {
-        // When an ES module has a syntax error, the resulting exception does not
-        // include the filename. Add it. We lose the stack trace in the process,
-        // but the stack trace is usually not useful since it contains only frames
-        // from the Node module loader.
-        const updatedError = new Error(`While loading ${path}: ${e.constructor.name}: ${e.message}`);
-        return Promise.reject(updatedError);
-      }
+      return Promise.reject(fixupImportException(e, path));
     });
   } else {
     return new Promise(resolve => {
@@ -38,3 +29,97 @@ function requireShim(path) {
 function importShim(path) {
   return import(path);
 }
+
+
+function fixupImportException(e, importedPath) {
+  // When an ES module has a syntax error, the resulting exception does not
+  // include the filename, which the user will need to debug the problem. We
+  // need to fix those up to include the filename. However, other kinds of load-
+  // time errors *do* include the filename and usually the line number. We need
+  // to leave those alone.
+  //
+  // Some examples of load-time errors that we need to deal with:
+  // 1. Syntax error in an ESM spec:
+  // SyntaxError: missing ) after argument list
+  //     at Loader.moduleStrategy (node:internal/modules/esm/translators:147:18)
+  //     at async link (node:internal/modules/esm/module_job:64:21)
+  //
+  // 2. Syntax error in an ES module imported from an ESM spec. This is exactly
+  // the same as #1: there is no way to tell which file actually has the syntax
+  // error.
+  //
+  // 3. Syntax error in a CommonJS module imported by an ES module:
+  // /path/to/commonjs_with_syntax_error.js:2
+  //
+  //
+  //
+  // SyntaxError: Unexpected end of input
+  //     at Object.compileFunction (node:vm:355:18)
+  //     at wrapSafe (node:internal/modules/cjs/loader:1038:15)
+  //     at Module._compile (node:internal/modules/cjs/loader:1072:27)
+  //     at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)
+  //     at Module.load (node:internal/modules/cjs/loader:988:32)
+  //     at Function.Module._load (node:internal/modules/cjs/loader:828:14)
+  //     at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:201:29)
+  //     at ModuleJob.run (node:internal/modules/esm/module_job:175:25)
+  //     at async Loader.import (node:internal/modules/esm/loader:178:24)
+  //     at async file:///path/to/esm_that_imported_cjs.mjs:2:11
+  //
+  // Note: For Jasmine's purposes, case 3 only occurs in  Node >= 14.8. Older
+  // versions don't support top-level await, without which it's not possible to
+  // load a CommonJS module from an ES module at load-time. The entire content
+  // above, including the file path and the three blank lines, is part of the
+  // error's `stack` property. There may or may not be any stack trace after the
+  // SyntaxError line, and if there's a stack trace it may or may not contain
+  // any useful information.
+  //
+  // 4. Any other kind of exception thrown at load time
+  //
+  //  Error: nope
+  //     at Object.<anonymous> (/path/to/file_throwing_error.js:1:7)
+  //     at Module._compile (node:internal/modules/cjs/loader:1108:14)
+  //     at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)
+  //     at Module.load (node:internal/modules/cjs/loader:988:32)
+  //     at Function.Module._load (node:internal/modules/cjs/loader:828:14)
+  //     at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:201:29)
+  //     at ModuleJob.run (node:internal/modules/esm/module_job:175:25)
+  //     at async Loader.import (node:internal/modules/esm/loader:178:24)
+  //     at async file:///path_to_file_importing_broken_file.mjs:1:1
+  //
+  // We need to replace the error with a useful one in cases 1 and 2, but not in
+  // cases 3 and 4. Distinguishing among them can be tricky. Simple heuristics
+  // like checking the stack trace for the name of the file we imported fail
+  // because it often shows up even when the error was elsewhere, e.g. at the
+  // bottom of the stack traces in the examples for cases 3 and 4 above. To add
+  // to the fun, file paths in errors on Windows can be either Windows style
+  // paths (c:\path\to\file.js) or URLs (file:///c:/path/to/file.js).
+
+  if (!(e instanceof SyntaxError)) {
+    return e;
+  }
+
+  const escapedWin = escapeStringForRegexp(importedPath.replace(/\//g, '\\'));
+  const windowsPathRegex = new RegExp('[a-zA-z]:\\\\([^\\s]+\\\\|)' + escapedWin);
+  const windowsUrlRegex = new RegExp('file:///[a-zA-z]:\\\\([^\\s]+\\\\|)' + escapedWin);
+  const anyUnixPathFirstLineRegex = /^\/[^\s:]+:\d/;
+  const anyWindowsPathFirstLineRegex = /^[a-zA-Z]:(\\[^\s\\:]+)+:/;
+
+  if (e.message.indexOf(importedPath) !== -1
+      || e.stack.indexOf(importedPath) !== -1
+      || e.stack.match(windowsPathRegex) || e.stack.match(windowsUrlRegex)
+      || e.stack.match(anyUnixPathFirstLineRegex)
+      || e.stack.match(anyWindowsPathFirstLineRegex)) {
+    return e;
+  } else {
+    return new Error(`While loading ${importedPath}: ${e.constructor.name}: ${e.message}`);
+  }
+}
+
+// Adapted from Sindre Sorhus's escape-string-regexp (MIT license)
+function escapeStringForRegexp(string) {
+  // Escape characters with special meaning either inside or outside character sets.
+  // Use a simple backslash escape when it’s always valid, and a `\xnn` escape when the simpler form would be disallowed by Unicode patterns’ stricter grammar.
+  return string
+    .replace(/[|\\{}()[\]^$+*?.]/g, '\\$&')
+    .replace(/-/g, '\\x2d');
+}

spec/fixtures/esm-importing-commonjs-syntax-error/intermediate.js

@@ -0,0 +1 @@
+require('./syntax_error');

spec/fixtures/esm-importing-commonjs-syntax-error/jasmine.json

@@ -0,0 +1,7 @@
+{
+  "spec_dir": ".",
+  "spec_files": [
+    "spec.mjs"
+  ],
+  "helpers": []
+}

spec/fixtures/esm-importing-commonjs-syntax-error/spec.mjs

@@ -0,0 +1 @@
+await import('./intermediate.js');

spec/fixtures/esm-importing-commonjs-syntax-error/syntax_error.js

@@ -0,0 +1 @@
+{

spec/fixtures/esm-indirect-error/jasmine.json

@@ -0,0 +1,7 @@
+{
+  "spec_dir": ".",
+  "spec_files": [
+    "spec.mjs"
+  ],
+  "helpers": []
+}

spec/fixtures/esm-indirect-error/spec.mjs

@@ -0,0 +1,3 @@
+import './throws.mjs';
+
+it('a spec', () => {});

spec/fixtures/esm-indirect-error/throws.mjs

@@ -0,0 +1 @@
+throw new Error('nope');

spec/fixtures/topLevelAwaitSentinel.mjs

@@ -0,0 +1 @@
+await Promise.resolve();

spec/integration_spec.js

@@ -89,6 +89,32 @@ describe('Integration', function () {
     expect(output).toContain('syntax_error.mjs');
   });
 
+  it('handles syntax errors from a CommonJS module loaded from an ESM spec properly', async function() {
+    try {
+      await import('./fixtures/topLevelAwaitSentinel.mjs');
+    } catch (e) {
+      if (e instanceof SyntaxError && e.message === 'Unexpected reserved word') {
+        pending('This Node version does not support top-level await');
+      } else if (e.message === 'Not supported') {
+        pending('This Node version does not support dynamic import');
+      } else {
+        throw e;
+      }
+    }
+
+    const {exitCode, output} = await runJasmine('spec/fixtures/esm-importing-commonjs-syntax-error', true);
+    expect(exitCode).toEqual(1);
+    expect(output).toContain('SyntaxError');
+    expect(output).toContain('syntax_error.js');
+  });
+
+  it('handles exceptions thrown from a module loaded from an ESM spec properly', async function() {
+    const {exitCode, output} = await runJasmine('spec/fixtures/esm-indirect-error', true);
+    expect(exitCode).toEqual(1);
+    expect(output).toContain('nope');
+    expect(output).toContain('throws.mjs');
+  });
+
   it('can configure the env via the `env` config property', async function() {
     const {exitCode, output} = await runJasmine('spec/fixtures/env-config');
     expect(exitCode).toEqual(0);

spec/loader_spec.js

@@ -70,24 +70,81 @@ function esModuleSharedExamples(extension, alwaysImport) {
     await expectAsync(loaderPromise).toBeResolved();
   });
 
-  it("adds the filename to errors that don't include it", async function() {
+  it("adds the filename to ES module syntax errors", async function() {
     const underlyingError = new SyntaxError('some details but no filename, not even in the stack trace');
-    const importShim = () => Promise.reject(underlyingError);
-    const loader = new Loader({importShim});
+    const loader = new Loader({importShim: () => Promise.reject(underlyingError)});
 
     await expectAsync(loader.load(`foo.${extension}`, alwaysImport)).toBeRejectedWithError(
       `While loading foo.${extension}: SyntaxError: some details but no filename, not even in the stack trace`
     );
   });
 
-  it('propagates errors that already contain the filename without modifying them', async function () {
-    const requireShim = jasmine.createSpy('requireShim');
+  it('does not modify errors that are not SyntaxError instances', async function() {
     const underlyingError = new Error('nope');
-    underlyingError.stack = underlyingError.stack.replace('loader_spec.js', `foo.${extension}`);
-    const importShim = jasmine.createSpy('importShim')
-      .and.callFake(() => Promise.reject(underlyingError));
-    const loader = new Loader({requireShim, importShim});
+    const loader = new Loader({importShim: () => Promise.reject(underlyingError)});
 
     await expectAsync(loader.load(`foo.${extension}`, alwaysImport)).toBeRejectedWith(underlyingError);
   });
+
+  it('does not modify SyntaxErrors that mention the imported filename as a Unix-style path', async function() {
+    const underlyingError = new SyntaxError('nope');
+    underlyingError.stack = `/the/absolute/path/to/foo.${extension}:1\n` +
+      '\n' +
+      '\n' +
+      '\n' +
+      'maybe some more stack\n';
+    const loader = new Loader({importShim: () => Promise.reject(underlyingError)});
+
+    await expectAsync(loader.load(`path/to/foo.${extension}`, alwaysImport))
+      .toBeRejectedWith(underlyingError);
+  });
+
+  it('does not modify SyntaxErrors that mention the imported filename as a Unix-style file URL', async function() {
+    const underlyingError = new SyntaxError('nope');
+    underlyingError.stack += `\n     at async file:///the/absolute/path/to/foo.${extension}:1:1`;
+    const loader = new Loader({importShim: () => Promise.reject(underlyingError)});
+
+    await expectAsync(loader.load(`path/to/foo.${extension}`, alwaysImport))
+      .toBeRejectedWith(underlyingError);
+  });
+
+  it('does not modify SyntaxErrors that mention the imported filename as a Windows-style path', async function() {
+    const underlyingError = new SyntaxError('nope');
+    underlyingError.stack = `c:\\the\\absolute\\path\\to\\foo.${extension}:1\n` +
+      '\n' +
+      '\n' +
+      '\n' +
+      'maybe some more stack\n';
+    const loader = new Loader({importShim: () => Promise.reject(underlyingError)});
+
+    await expectAsync(loader.load(`path/to/foo.${extension}`, alwaysImport))
+      .toBeRejectedWith(underlyingError);
+  });
+
+  it('does not modify SyntaxErrors that mention the imported filename as a Windows-style file URL', async function() {
+    const underlyingError = new SyntaxError('nope');
+    underlyingError.stack += `\n     at async file:///c:/the/absolute/path/to/foo.${extension}:1:1`;
+    const loader = new Loader({importShim: () => Promise.reject(underlyingError)});
+
+    await expectAsync(loader.load(`path/to/foo.${extension}`, alwaysImport))
+      .toBeRejectedWith(underlyingError);
+  });
+
+  it('does not modify SyntaxErrors when the stack trace starts with any Unix-style path', async function() {
+    const underlyingError = new SyntaxError('nope');
+    underlyingError.stack = '/some/path/to/a/file.js:1\n\n\n\n' + underlyingError.stack;
+    const loader = new Loader({importShim: () => Promise.reject(underlyingError)});
+
+    await expectAsync(loader.load(`path/to/some/other/file.${extension}`, alwaysImport))
+      .toBeRejectedWith(underlyingError);
+  });
+
+  it('does not modify SyntaxErrors when the stack trace starts with any Windows-style path', async function() {
+    const underlyingError = new SyntaxError('nope');
+    underlyingError.stack = 'c:\\some\\path\\to\\a\\file.js:1\n\n\n\n' + underlyingError.stack;
+    const loader = new Loader({importShim: () => Promise.reject(underlyingError)});
+
+    await expectAsync(loader.load(`path/to/some/other/file.${extension}`, alwaysImport))
+      .toBeRejectedWith(underlyingError);
+  });
 }