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

Fix: wrong error thrown while loading reporter #4842

Merged
merged 3 commits into from
Mar 11, 2022

Conversation

juergba
Copy link
Contributor

@juergba juergba commented Mar 9, 2022

Description

When developing a third-party reporter, Mocha misdiagnoses an error coming from within the reporter as the reporter module not being found.

Description of the Change

  • lib/cli/run-helpers.js: fix CLI plugin entry point
  • lib/mocha.js: fix/simplify reporter() when used programmatically via Mocha API
  • adapt existing tests
  • adapt webpack configuration

Applicable issues

closes #3596

@juergba juergba self-assigned this Mar 9, 2022
@juergba juergba added area: node.js command-line-or-Node.js-specific area: reporters involving a specific reporter semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Mar 9, 2022
@juergba juergba added this to the next milestone Mar 9, 2022
@juergba juergba marked this pull request as ready for review March 9, 2022 16:52
@juergba juergba requested a review from a team March 9, 2022 16:52
@juergba juergba merged commit 241964b into master Mar 11, 2022
@juergba juergba deleted the juergba/require_reporter branch March 11, 2022 08:41
try {
reporter = require(reporterName);
foundReporter = require.resolve(reporterName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juergba this line caused a warning when trying to bundle mocha/mocha.js using webpack

Copy link
Contributor Author

@juergba juergba Mar 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AviVahl This one: Critical dependency: the request of a dependency is an expression?
Is this a problem? I need to check wether the module exists, and I can't use a fs function either.
In a browser environment we shouldn't get up to this line anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly that one.
The warning actually broke several internal projects we have that bundle mocha into the browser. There was infra code there that verified no errors/warnings from bundling.

If the branch is node-specific, would it be possible to completely separate it out of the browser bundle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I achieve this, exclude a specific code block from the bundle?

Btw the transpiled ES5 bundle (mocha.js) will be dropped in a few weeks, and the ES2018 bundle (mocha-es2018.js renamed to mocha.js) will be the one and only. If you have any input, would be welcome.

Copy link
Contributor

@AviVahl AviVahl Mar 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can think of several possible ways to achieve this.

I usually separate the isomorphic parts of code from the Node-specific parts.
So: the cli, config loading, reporter loading, dynamic require/require.resolve, etc would all belong to the Node-specific part.
The mocha.js bundle would only include the runner itself and associated APIs.

Another, perhaps trickier, way to achieve this is to specify "browser": { "./my-node-specific-file.js": null} which would cause bundlers to skip that file and inject an empty object instead. Runtime checks would be needed to protect against the replaced API.

Regarding es2018? My team welcomes this change. :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue is present even now in v10. Have you found a solution @AviVahl ? I can supress these errors in webpack config but it doesn't seem like the right solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've stopped bundling mocha from our end because of the many bundling regressions it exhibited across the years/versions. Our tool (mocha-play) uses the prebuilt bundle (in the npm package) and injects it to the page.

KuznetsovRoman pushed a commit to gemini-testing/mocha that referenced this pull request Sep 6, 2022
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: reporters involving a specific reporter semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Correctly diagnose errors from required reporter module
3 participants