fix: fully support 'extends' for configurations

[mf-pherlihy]

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• All new code requires tests to ensure against regressions.

Description of the Change

There have been multiple issues reported where configurations do not actually support the extends property:

https://github.com/mochajs/mocha/issues/3916 https://github.com/mochajs/mocha/pull/4057

I did some digging and found the root cause to be the options.parse() method. This method relies on yargs-parser and NOT yargs which does the actual configuration extending.

It was a pretty straightforward fix. I imported the yargs function that extends the configuration objects and then applied it to all the arguments passed to parse().

I added a test to support this case and verified the test fails when this fix is not applied.

Alternate Designs

There was already an existing PR open that modified external methods to do this, so I wanted to avoid that.

Why should this be in core?

It fixes a long standing bug.

Benefits

Configuration files can now be extended properly.

Possible Drawbacks

I dont see any?

Applicable issues

[jsf-clabot]

All committers have signed the CLA.

[boneskull]

Yeah, I think that test should probably live in test/node-unit/cli/options.spec.js instead.

[bcoe]

I'm a little concerned that yargs/lib/apply-extends isn't a public API. If it isn't, maybe we can vendor it (e.g., just copy/paste it into a function in lib/cli/options.js). @bcoe can you comment?

I would be happy to make this a public API, but as this PR stands today, it will be broken by the next major version of yargs, which moves to using an export map.

I'd be happy to export this helper though.

[boneskull]

@bcoe If you want to add it to the export map, that'd work for us--we can change our code to use it when we upgrade.

[boneskull]

@mf-pherlihy Are you interested in doing further work on this? I'd like to merge the PR, but the test should probably move. If I don't hear back in a couple days, I'll merge this as-is and fix it after.

[mf-pherlihy]

@boneskull Yeah I can finish this up. Just to confirm:

1 . Move to test/node-unit/cli/options.spec.js 2a. Copy applyExtends from yargs to here with a TODO to swap out once yargs is updated? 2b. Use applyExtends as-is with a TODO to swap out once yargs is updated?

[boneskull]

1. Move the test
2. AFAICT you are free to use applyExtends as it has been made part of yargs' public API. But it sounds like when we upgrade yargs we will need to change it? Unclear, since we're pulling it out of yargs-parser and not yargs itself, right? @bcoe?

[boneskull]

ping @bcoe @mf-pherlihy

[bcoe]

AFAICT you are free to use applyExtends

Yes, my intention was that it's a blessed public helper now. The caveat is that we need to fix this.

[mf-pherlihy]

Just tried upgrading yargs and using the defined helpers. Unfortunately however, the latest version breaks a bunch of other tests.

[coveralls]

Coverage increased (+0.01%) to 94.157% when pulling 105d1aa8f4179278625409e7bdec98e4485d0232 on mf-pherlihy:extends-fix into 30d5b667a0b877a62ad33a49edc72807dccd768d on mochajs:master.

[bcoe]

@mf-pherlihy [email protected] introduces ESM, and was a somewhat major shuffling around of code. I'd be interested in making sure we address any issues you're bumping into with tests.

[juergba]

@bcoe I did upgrade yargs and yargs-parser with #4543. There have been changes in yargs' array handling with yargs.config() and combinig array values. I had to remove aliased options from our pre-parsed result (by yargs-parser) in order to prevent double entries. I do not consider the new behavior as a bug, it seems more consistent now than before.

[bcoe]

I do not consider the new behavior as a bug, it seems more consistent now than before.

@juergba that's good to hear 👍

Sound like we perhaps just need to rebase this and give the helpers another try?

[juergba]

Sound like we perhaps just need to rebase this and give the helpers another try?

I guess so, but I haven't taken a look at this PR though. But please go ahead @mf-pherlihy. The browser tests will fail anyway, but that's off topic. Github actions does not have access to GH secrets (login data for SauceLabs) on PR's from forked repos. We haven't solved that, yet.

[juergba]

@mf-pherlihy I rebased and tests are passing with yargs public helpers.

[juergba]

Priority list:
 * 1. Command-line args
 * 2. RC file (`.mocharc.c?js`, `.mocharc.ya?ml`, `mocharc.json`)
 * 3. `mocha` prop of `package.json`
 * 4. default configuration (`lib/mocharc.json`)

I have to check to which configObjects you are applying extends. To RC files only or to package.json mocha field as well. And when the extends values are applied, wether the priority is respected. I'm unsure wether this is correct.

[mf-pherlihy]

@mf-pherlihy I did some testing and have found quite a few issues:

• extends does not work for relative paths
• array typed options: values are not combined, the configuration of yargs-parser seems to be ignored by applyExtends.
• boolean typed option: is buggy, eg no-diff: true gets overwritten by default value: diff: true
• package.json with field mocha: haven't tested yet, but ?

As an alternative without yargs public helper: we just let options.parse() do the job by passing additional configObjects. The order would be crucial. configObjects: [{base RC-options}, {extends RC-options}, {base pkg options}, {extends pkg options}]

What do you think?

If we can cover all the issues with yargs-parser and still fix the root issue with extends then im all for it.

[bcoe]

@mf-pherlihy if any of this behavior would make sense in yargs/yargs-parser, happy to take patches.

[SmashingQuasar]

:wave: Where are we at regarding this PR? Extending configuration is essential to properly manage mono-repositories. If you need any help to finish it just say the word and I'll happily look into whatever issue remains. :+1:

[JoshuaKGoldberg]

Note: #4980 also touches this area. We should review them together in case one is a dup.

[JoshuaKGoldberg]

Marking as semver-major because some projects might accidentally be relying on the current, arguably broken behavior.