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 new option "node-option" #4691

Merged
merged 5 commits into from
Jul 31, 2021
Merged

Add new option "node-option" #4691

merged 5 commits into from
Jul 31, 2021

Conversation

juergba
Copy link
Contributor

@juergba juergba commented Jul 19, 2021

Description

Currently Mocha is parsing its own options, plus Node and V8 options as well. Mocha has to recognize those foreign options in order to pass them to a newly spawned child-process.

Mocha's options are defined in yargs/yargs-parser, whereas Node/V8 options:

  • are not white-listed
  • Node: compared with process.allowedNodeEnvironmentFlags, which contains most but not all flags.
  • V8: compared against a beautiful regex, otherwise v8[_-] has to be prepended.

This Mocha/Node hybrid parsing has some disadvantages:

  • maintenance overhead
  • some options can't be forwarded at all, eg. --prof, --require
  • Mocha's option object gets polluted with invalid camelCased Node/V8 options
  • Mocha can't print a warning in case of incorrect user input (typos)
  • we have bugs in parsing of our own kebab-case options, which are difficult to fix with the current yargs-parser configuration

Description of the Change

Mocha should not have to check/process any Node/V8 flags, but simply parse and hand them over to Node.
So we add a new flag --node-option/ -n:

  • array:
    • -n require=glob -n unhandled-rejections=strict or
    • -n require=glob,unhandled-rejections=strict
  • no checks or processing, Mocha just hands the option over to Node by spawning a new child-process
  • no merging: the two solutions are independent
  • optional: remove current Mocha/Node hybrid solution on mid-term

in addition:

  • prevent double parsing when bin/mocha is the entry point
  • remove package esm special handling. This package is abandoned and not supported in Mocha v9
  • remove gc check and deprecation warning. This V8 option doesn't exist anymore

@juergba juergba force-pushed the juergba/node-option branch 2 times, most recently from baf4095 to fd95c88 Compare July 25, 2021 11:49
@juergba juergba added area: node.js command-line-or-Node.js-specific semver-minor implementation requires increase of "minor" version number; "features" area: usability concerning user experience or interface labels Jul 25, 2021
@juergba juergba marked this pull request as ready for review July 25, 2021 12:15
@juergba juergba requested a review from a team July 25, 2021 12:15
@juergba juergba self-assigned this Jul 25, 2021
@juergba juergba force-pushed the juergba/node-option branch from fd95c88 to 1521b78 Compare July 27, 2021 15:57
Copy link
Contributor

@outsideris outsideris left a comment

Choose a reason for hiding this comment

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

👍🏼


> _New in v9.1.0._

For Node.js and V8 options. Mocha forwards these options to Node.js by spawning a new child-process.<br>
Copy link
Contributor

Choose a reason for hiding this comment

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

To clear, can we specify name should be without -- nor --v8-?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the -- info. I prefer to not mention --v8- as this is a Mocha specific quirk and may be confusing.

@@ -176,6 +176,10 @@ exports.builder = yargs =>
group: GROUPS.OUTPUT,
hidden: true
},
'node-option': {
description: 'Node or V8 option (no leading "--")',
Copy link
Contributor

Choose a reason for hiding this comment

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

How about no leading "--" or "--v8-"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't added the --v8- part, see comment above.

@juergba juergba force-pushed the juergba/node-option branch from 1521b78 to ee2203c Compare July 30, 2021 12:48
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-minor implementation requires increase of "minor" version number; "features"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants