Dynamic import does not show location of SyntaxError

[coreyfarrell]

// syntax-error.mjs
console.log([
  'str1'
  'str2'
]);

Perform a dynamic import with node -e "import('./syntax-error.mjs').catch(console.error)", the output does not help identify the error and could be seen by users as if node.js itself had a SyntaxError:

SyntaxError: Unexpected string
    at Loader.moduleStrategy (internal/modules/esm/translators.js:83:18)
    at async link (internal/modules/esm/module_job.js:37:21)

Now create and run a loader.mjs script:

import './syntax.mjs';

Running loader.mjs produces useful output:

file:///usr/src/npm/failures/syntax-error.mjs:3
	'str2'
	^^^^^^

SyntaxError: Unexpected string
    at Loader.moduleStrategy (internal/modules/esm/translators.js:83:18)

I was not able to find a way to catch the error of a dynamic import and show the complete SyntaxError. Interestingly node --unhandled-rejections=strict -e "import('./syntax-error.mjs')" or running await import('./syntax-error.mjs') from node --experimental-repl-await both display the location of the SyntaxError.

[hybrist]

This is something we unfortunately inherited from Chrome/V8. You can try the following in the browser devtools:

await import('data:text/javascript,<not valid syntax>')

We may be able to patch the stack even further. The main issue is that import() actually makes these errors easily observable. So the fact that err.stack is "weird" and doesn't properly start with the usual prefix would leak.

[ljharb]

That leak will pose a problem as the error stacks proposal attempts to advance; it would be exceedingly ideal to avoid creating new observable stack trace formats/contents.

[hybrist]

One thing I'd love to see is to have syntax errors from module executions include the parsed module as the top frame. But I can understand why that may be awkward (there's no actual stack frame running that code).

[ljharb]

My primary concern is avoiding new kinds of traces; id rather see no error than a new kind of error. A very close second is seeing useful ones :-)

[hybrist]

Somewhat OT: The above stack traces are a great reason to invest into moving even more of the loader into C++. None of these frames are remotely helpful to understand what's going on:

SyntaxError: Unexpected string
    at Loader.moduleStrategy (internal/modules/esm/translators.js:83:18)
    at async link (internal/modules/esm/module_job.js:37:21)

Compared with the - theoretical - error showing the syntax error location as the top frame and without any JS frames from loader internals:

SyntaxError: Unexpected string
    at file:///path/to/project/syntax-error.mjs:4:2

[addaleax]

@jkrems If the issue is that there are C++ frames in the middle of a stack, cutting it off at some point, I’d prefer to come up with a solution that provides the full stack trace rather than moving more code into C++…

[hybrist]

I think to me it's related to a direct comparison with browser stacks which is "suddenly" a thing for modules. With most other APIs (from setTimeout() to require()), there's user code calling JavaScript functions. So seeing JS frames of node's implementation doesn't feel too weird to me.

But module execution feels different. I didn't create a module job, I ran a file. In the browser, it wouldn't show me stack frames from network requests and other kinds of orchestration needed to run the file. So seeing that in node appears inconsistent.

[addaleax]

@jkrems Right, but I feel like “move code to a different language” is ultimately not a good solution to stack traces not being pretty enough – I’d rather find a way to provide better stack traces. In my opinion, we already have far too much module-related C++ code that would be more accessible when written in JS.

[GeoffreyBooth]

It’s always annoyed me that Node internals appear in stack traces. I wish we could just filter them out, or blackbox them, maybe by default which could be changed via a flag.

[hybrist]

@addaleax That's definitely fair. Maybe what's missing is some API that allows us to start module execution with a "fresh stack" even when there's technically active JS frames. Feels like something that'd need V8 support if they would also appear "properly" within the module execution itself (try/catch)..?

It’s always annoyed me that Node internals appear in stack traces. I wish we could just filter them out, or blackbox them, maybe by default which could be changed via a flag.

@GeoffreyBooth I generally disagree. Seeing frames for the express subclass of http.Server but not for the parts that happen to be from the base class feels super confusing. In most cases having node's own code appear is pretty valuable imo.

[ljharb]

The function implementation hiding proposal, which allows functions to opt out of stack frames, may provide a JS-native solution to this problem.

[hybrist]

The function implementation hiding proposal, which allows functions to opt out of stack frames, may provide a JS-native solution to this problem.

Maybe. Though it feels a bit brittle. I assume it would mean that we'd have to manually hide each individual function that makes up our implementation and be careful that we never have Array.forEach & friends on the stack. Possible but a bit too easy to break by accident for my taste.

[coreyfarrell]

I can understand arguments for and against node.js internals appearing in the stack, but that's not why I opened this issue. My concern is not so much the extra information, it's the lack of necessary information. Filtering out node.js internals will not tell me where the syntax error occurred. Normally I use a linter but in some cases when testing a specific issue I might run the tests manually and bypass the lint step (that's how I found this issue).

I know @jkrems suggested that this could be a v8 issue but I find it interesting that REPL top-level await and strict unhandled rejections both display the necessary information. This tells me that the information exists as part of the error object at some point. Seems like that information can only be displayed by the default uncaughtException handler?

function example() {
  throw new Error('example');
}

if (process.env.CATCH_ERROR) {
  try {
    example();
  } catch (error) {
    console.error(error);
  }
} else {
  example();
}

Running this with CATCH_ERROR=1 removes the source output though in this example it's less of an issue as the stack trace contains a line referencing the location of throw in example().

[hybrist]

Yes, it can “always” appear on crashes and that’s definitely a good improvement. The issue is that it won’t appear on things that don’t crash or where errors are sent to reporting tools (e.g. log files). Making it part of the default unhandled rejection logging is a good quick fix though!