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

node flag parsing strategy is backwards #4417

Closed
boneskull opened this issue Aug 25, 2020 · 3 comments · Fixed by #4418
Closed

node flag parsing strategy is backwards #4417

boneskull opened this issue Aug 25, 2020 · 3 comments · Fixed by #4418
Assignees
Labels
type: bug a defect, confirmed by a maintainer

Comments

@boneskull
Copy link
Contributor

nodejs/node#34637 introduces a -u flag for node, which conflicts with Mocha's -u/--ui flag. I had expected Mocha would prefer its own options over node's where there was a conflict, but this is not the case--instead, Mocha explicitly whitelists -r/--require, which was previously the only known command-line flag shared between mocha and node.

To fix this, Mocha needs to change its strategy to detect its own options first, then attempt to detect if something is a node option. Currently, Mocha checks for node options first.

@boneskull boneskull added the type: bug a defect, confirmed by a maintainer label Aug 25, 2020
@boneskull boneskull self-assigned this Aug 25, 2020
@boneskull
Copy link
Contributor Author

cc @BethGriggs

@boneskull
Copy link
Contributor Author

This does mean that if someone wants to use custom conditions w/ mocha, they need to use --conditions, not -u.

boneskull added a commit that referenced this issue Aug 25, 2020
- no more `require` special-case
- future-proofs `mocha` against the introduction of conflicting flags in `node`
@boneskull
Copy link
Contributor Author

Fix is in #4418

boneskull added a commit that referenced this issue Aug 25, 2020
- no more `require` special-case
- future-proofs `mocha` against the introduction of conflicting flags in `node`
MylesBorins pushed a commit to nodejs/node that referenced this issue Aug 26, 2020
Old versions of mocha break after #34637.

This was a bug in mocha, but since this is a widely used module
we can expect ecosystem breakage until modules are updated to
the latest version of mocha. Drop the conflicting `-u` alias --
we can potentially bring it back once modules have been updated.

PR-URL: #34935
Refs: mochajs/mocha#4417
Refs: #34637
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Aug 26, 2020
Old versions of mocha break after #34637.

This was a bug in mocha, but since this is a widely used module
we can expect ecosystem breakage until modules are updated to
the latest version of mocha. Drop the conflicting `-u` alias --
we can potentially bring it back once modules have been updated.

PR-URL: #34935
Refs: mochajs/mocha#4417
Refs: #34637
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
guybedford pushed a commit to guybedford/node that referenced this issue Sep 28, 2020
Old versions of mocha break after nodejs#34637.

This was a bug in mocha, but since this is a widely used module
we can expect ecosystem breakage until modules are updated to
the latest version of mocha. Drop the conflicting `-u` alias --
we can potentially bring it back once modules have been updated.

PR-URL: nodejs#34935
Refs: mochajs/mocha#4417
Refs: nodejs#34637
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this issue Oct 1, 2020
Old versions of mocha break after #34637.

This was a bug in mocha, but since this is a widely used module
we can expect ecosystem breakage until modules are updated to
the latest version of mocha. Drop the conflicting `-u` alias --
we can potentially bring it back once modules have been updated.

PR-URL: #34935
Backport-PR-URL: #35385
Refs: mochajs/mocha#4417
Refs: #34637
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant