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

Add file location when SyntaxError happens in ESM (fixes #4551) #4557

Merged
merged 3 commits into from Feb 3, 2021

Conversation

giltayar
Copy link
Contributor

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)

@coveralls
Copy link

coveralls commented Jan 20, 2021

Coverage Status

Coverage decreased (-0.02%) to 94.126% when pulling fb77b31 on giltayar:esm-syntax-error into 84d0c96 on mochajs:master.

@giltayar
Copy link
Contributor Author

I'm guessing the coverage decreased because I didn't add a test. Should I?

@giltayar
Copy link
Contributor Author

giltayar commented Jan 20, 2021

I added a test and coverage still decreased, so I'm guessing the coverage is not running on the ESM code?

@juergba
Copy link
Member

juergba commented Jan 20, 2021

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 outsideris added type: bug a defect, confirmed by a maintainer area: usability concerning user experience or interface labels Jan 20, 2021
lib/esm-utils.js Show resolved Hide resolved
@juergba
Copy link
Member

juergba commented Feb 2, 2021

I restarted our CI tests, because I have problems with GH actions on another PR.
Yes, now your PR does not pass anymore. Without any changes, before it passed, now the ESLint tests fail.
I hate GH actions, it's just so unreliable and bitchy. Tomorrow is another day, we will see.

@juergba
Copy link
Member

juergba commented Feb 3, 2021

It's even worse now, the tests are now failing due to some parallel problem. And the ESLint failure persists.
Every time I restart this GH action it's getting worse.

@juergba
Copy link
Member

juergba commented Feb 3, 2021

@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.
Could you have a look, please?

@juergba
Copy link
Member

juergba commented Feb 3, 2021

I will test an update of workerpool dependency. On this branch if you don't mind.

Edit: upgrade was successful.

@giltayar
Copy link
Contributor Author

giltayar commented Feb 3, 2021

@juergba Looks like it did the trick.

@juergba
Copy link
Member

juergba commented Feb 3, 2021

@giltayar do we merge?

@giltayar
Copy link
Contributor Author

giltayar commented Feb 3, 2021

@juergba LGTM

@juergba juergba merged commit 9878f32 into mochajs:master Feb 3, 2021
@juergba juergba added the area: node.js command-line-or-Node.js-specific label Feb 3, 2021
@juergba juergba added this to the next milestone Feb 3, 2021
@juergba juergba added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Feb 3, 2021
@giltayar giltayar deleted the esm-syntax-error branch February 8, 2021 06:08
@juergba juergba modified the milestones: next, v8.3.0 Feb 11, 2021
This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific area: usability concerning user experience or interface semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants