Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic import does not show location of SyntaxError #49441

Open
coreyfarrell opened this issue Jan 16, 2020 · 34 comments
Open

Dynamic import does not show location of SyntaxError #49441

coreyfarrell opened this issue Jan 16, 2020 · 34 comments
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. promises Issues and PRs related to ECMAScript promises.

Comments

@coreyfarrell
Copy link
Member

// 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.

@jkrems
Copy link
Contributor

jkrems commented Jan 16, 2020

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
Copy link
Member

ljharb commented Jan 16, 2020

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.

@jkrems
Copy link
Contributor

jkrems commented Jan 16, 2020

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
Copy link
Member

ljharb commented Jan 16, 2020

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 :-)

@jkrems
Copy link
Contributor

jkrems commented Jan 16, 2020

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
Copy link
Member

@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++…

@jkrems
Copy link
Contributor

jkrems commented Jan 16, 2020

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
Copy link
Member

@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
Copy link
Member

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.

@jkrems
Copy link
Contributor

jkrems commented Jan 16, 2020

@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
Copy link
Member

ljharb commented Jan 16, 2020

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

@jkrems
Copy link
Contributor

jkrems commented Jan 16, 2020

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
Copy link
Member Author

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().

@jkrems
Copy link
Contributor

jkrems commented Jan 20, 2020

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!

@benbucksch
Copy link
Contributor

benbucksch commented May 23, 2020

This bug is a big obstacle for me during development. I need to load apps dynamically, so any syntax error in any of my code will now not show anymore. It's very difficult to find errors without knowing where exactly the error is. This is completely unworkable.

My workaround is to manually add hardcoded import statements to the files that I develop on at a given time. But that's cumbersome, tedious, and I need to do it manually every time. All the time. And this is only after I found the cause of why the stack is wrong. Many other developers would not even know why the stack is mis-attributing and would just tear their hairs without ever knowing why it failed.

As @coreyfarrell said, because stacks are so important, even async/await processing impl found some way to preserve the stack, so I assume there must be some kind of workaround possible for this problem as well.

This bug here is a serious problem for any usage of import().

@ghost
Copy link

ghost commented Oct 25, 2020

I stumbled upon a scenario where one of my package would dynamically import files and run whatever methods was exported as you can guess I would be having a hard time figuring out where the error comes from when this:

SyntaxError: Unexpected reserved word
    at Loader.moduleStrategy (node:internal/modules/esm/translators:143:18)
    at async link (node:internal/modules/esm/module_job:42:21)

is the only info I am getting.

@egasimus
Copy link

This is still the case. Any workarounds for v14.16.0, or is this fixed in v15?

@FossPrime
Copy link

This is still the case. Any workarounds for v14.16.0, or is this fixed in v15?

If its just a syntax error... running eslint will catch the issue clearly. Most of this issue is gone... the remaining issue is with Dynamic Imports... which packages like Mocha use. But they worked around it a barely, they only tell you what test doesn't load, not what file caused the error. ESLint really helps, and found the issue with accuracy.

@egasimus
Copy link

So, still broken.

@FossPrime
Copy link

Or rather, working as intended... which is much faster, but if you are doing dynamic imports... much less clear. Oof, I just realized I should handle my new dynamic imports a bit more clearly. Thanks for the reminder.

@egasimus
Copy link

egasimus commented May 11, 2021

So, still broken.

Or rather, working as intended...

So, NOTABUG WONTFIX then?

@benbucksch
Copy link
Contributor

benbucksch commented May 11, 2021

(For those who missed the sarcasm, I presume that egasimus' last comment was ironic.)

@rayfoss I'm glad that you found a workaround that works for you. However, this is a serious problem for some of us. Your workaround doesn't work for everybody, for various reasons. Diminishing the importance of a bug, after others have expressed that this is a serious problem for them, is generally not a good idea and not productive, and it only serves to create a useless debate without any helpful result. Please refrain from comments like your last comment.

@FossPrime
Copy link

Places where this is still a problem:

  • If you are using an old version of Mocha, it uses Dynamic imports, but in the latest stable it catches the error and explains what was trying to do.
  • If you pipe code into the Node REPL. Currently the best workaround is to import in a loop and explain the error yourself.
  • Any other place where you would use dynamic imports

@egasimus
Copy link

egasimus commented Jun 2, 2021

@rayfoss I started using Mocha today. Here's what 8.4.0 does:

Screenshot_20210602_080505

@giltayar
Copy link
Contributor

giltayar commented Jun 2, 2021

@egasimus this is because we added coded in Mocha specifically to overcome this error. It's an ugly hack, and we put it there because people were complaining about this, so this is not a theoretical problem. It is out there.

Hack: https://github.com/mochajs/mocha/blob/master/lib/esm-utils.js#L14

@egasimus
Copy link

egasimus commented Jun 2, 2021

@giltayar I'm fairly certain the syntax error is not in ██████.spec.js at all, but in one of its dependencies. So this workaround does point to the file that triggered the error, but not to the file that actually contains the error. (Not to mention it does not include essential info such as line and column.)

Trying to load the module from the REPL with require says I must use import; therefore, the Node.js runtime gives me no way to find SyntaxError in my code. This is a fairly significant regression and I'm surprised it hasn't gotten more attention.

@giltayar
Copy link
Contributor

giltayar commented Jun 2, 2021

@egasimus Totally agree! We did what we could to alleviate the pain, but the pain is there even after our workaround.

@vieiramanda11
Copy link

Still facing this issue with Mocha:

> mocha


SyntaxError[ @/Users/amandavieira/Documents/personal-projects/inbox-eth/test/Inbox.test.js ]: Unexpected reserved word
    at Loader.moduleStrategy (internal/modules/esm/translators.js:140:18)
    at async link (internal/modules/esm/module_job.js:42:21)

Any idea how can I solve this?

@forivall
Copy link
Contributor

forivall commented Nov 1, 2021

#49441 - file was moved, link is now
https://github.com/mochajs/mocha/blob/master/lib/nodejs/esm-utils.js

gamemaker1 referenced this issue in express-rate-limit/express-rate-limit Dec 28, 2021
- it gives this weird error:

  SyntaxError: Unexpected reserved word
    at Loader.moduleStrategy (internal/modules/esm/translators.js:140:18)
    at async link (internal/modules/esm/module_job.js:42:21)

- found these issues; should debug further:
	- nodejs/node#32177
	- https://github.com/nodejs/modules/issues/471
gamemaker1 referenced this issue in express-rate-limit/express-rate-limit Dec 30, 2021
- it gives this weird error:

  SyntaxError: Unexpected reserved word
    at Loader.moduleStrategy (internal/modules/esm/translators.js:140:18)
    at async link (internal/modules/esm/module_job.js:42:21)

- found these issues; should debug further:
	- nodejs/node#32177
	- https://github.com/nodejs/modules/issues/471
gamemaker1 referenced this issue in express-rate-limit/express-rate-limit Dec 30, 2021
- it gives this weird error:

  SyntaxError: Unexpected reserved word
    at Loader.moduleStrategy (internal/modules/esm/translators.js:140:18)
    at async link (internal/modules/esm/module_job.js:42:21)

- found these issues; should debug further:
	- nodejs/node#32177
	- https://github.com/nodejs/modules/issues/471
@Turbine1991

This comment was marked as abuse.

@egasimus
Copy link

egasimus commented Jun 28, 2023

Yikes, how is this still not resolved. This is of paramount importance in the node world.

✔️

This comment was marked as abuse.

🤦

There's a vast difference between "emotionally highlighting the importance of something that is frustrating" and "abuse".

Please take a moment to consider this distinction.

@Turbine1991's comment is clearly the former; marking it as "abuse" actually borders on the latter (gaslighting).

I understand that nobody is entitled to maintainers' time, and impacted users are always free to solve the issue themselves and submit a pull request.

However, contributing to a large and critical project such as Node.js is outside most downstream developers' comfort zone; the least the Node.js team could do is acknowledge this and not dismiss honest reactions as "abuse".

@benbucksch
Copy link
Contributor

This is of paramount importance in the node world.

This bug is so bad that I have to warn third party developers in our documentation about it. (See end of page https://docs.parula.app/develop/app/create-the-stub-files )

@GeoffreyBooth
Copy link
Member

This bug is so bad that I have to warn third party developers in our documentation about it.

You could also submit a pull request to fix it!

@GeoffreyBooth GeoffreyBooth transferred this issue from nodejs/modules Sep 1, 2023
@GeoffreyBooth GeoffreyBooth added promises Issues and PRs related to ECMAScript promises. errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. labels Sep 1, 2023
@adrian-balan-mindit
Copy link

any status on this bug ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

No branches or pull requests