Add file location when SyntaxError happens in ESM (fixes #4551) #4557
Conversation
|
I'm guessing the coverage decreased because I didn't add a test. Should I? |
|
I added a test and coverage still decreased, so I'm guessing the coverage is not running on the ESM code? |
|
Coverage does run, without test it was -0.1, now with test -0.02. If you look at the coverage details, you see line 27 (throw err) is the evil, newly added and uncovered line. For my taste your test is sufficiant. The browser test does not work on PR's raised from forked repos. |
outsideris
added
type: bug
|
I restarted our CI tests, because I have problems with GH actions on another PR. |
|
It's even worse now, the tests are now failing due to some parallel problem. And the ESLint failure persists. |
|
@giltayar I fixed the ESLint problem. GH does not delete expired artifacts, which results in failures whenever an app is trying to download one of those. I deleted all expired artifacts and scheduled an action for this purpose as well. But other tests are failing. Could it be the Node version? With Node 15.7. the tests passed, now with 15.8. they fail. |
|
I will test an update of Edit: upgrade was successful. |
juergba
force-pushed
the
esm-syntax-error
branch
from
c397318 to
fb77b31
Compare
|
@juergba Looks like it did the trick. |
|
@giltayar do we merge? |
|
@juergba LGTM |
juergba
added
the
semver-patch
Description of the Change
According to issue #4551 (I verified that it is true), when there is a syntax error in an ESM file, the error thrown by Node.js contains only the error, but not the location (file and line number) where this problem happened.
While the error doesn't include this information, we have the filename and can "inject" it into the error artificially. At least until Node.js fixes the problem. This change is local to ESM as it is only in the ESM importing code.
Alternate Designs
The only alternative I can see is to get Node.js to fix the problem.
Why should this be in core?
Because plugins can't fix this.
Benefits
It is currently difficult to figure out on which test file the syntax error happened. This helps a lot by at least giving the file where the syntax error is. Usually, in the IDE, the developer can figure out what the problem is.
Possible Drawbacks
The code that "patches" the error is convoluted and can theoretically fail in weird ways. Also, we won't know when Node.js fixes the problem, so we may have both indications on location in the error once Node.js fixes it.
Applicable issues
This is a bug fix (patch release)