fix: fully support 'extends' for configurations #4407

mf-pherlihy · 2020-08-13T01:20:41Z

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:

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 · 2020-08-13T01:20:43Z

All committers have signed the CLA.

boneskull

great work, thanks! 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?

outsideris

The test seems unit test rather than integration tests. So, test/node-unit/cli/ is better place where we put the test in. What do you think?

boneskull · 2020-08-25T19:12:01Z

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

bcoe · 2020-08-28T03:55:30Z

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 · 2020-08-28T22:34:02Z

@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 · 2020-08-28T22:34:57Z

@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 · 2020-08-31T20:48:25Z

@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 · 2020-09-09T23:48:22Z

Move the test
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 · 2020-11-02T22:36:59Z

ping @bcoe @mf-pherlihy

bcoe · 2020-11-03T15:29:08Z

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 · 2021-01-17T19:00:07Z

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

coveralls · 2021-01-17T19:12:08Z

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

bcoe · 2021-01-27T16:57:21Z

@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 · 2021-01-29T10:25:48Z

@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 · 2021-01-29T17:43:53Z

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 · 2021-01-29T18:37:33Z

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 · 2021-02-13T10:55:10Z

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

juergba · 2021-02-13T14:42:52Z

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.

juergba

@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?

mf-pherlihy · 2021-02-23T14:56:21Z

@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 · 2021-03-12T23:08:33Z

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

SmashingQuasar · 2023-05-08T09:53:25Z

👋 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. 👍

JoshuaKGoldberg · 2024-01-18T16:43:41Z

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

JoshuaKGoldberg

Seems like a reasonable start - and I like the test case!

I'm interested in what you think of this approach compared to #4980's. If possible, it'd be nice to avoid having to reach into lower-level helpers. But I'm not super familiar with yargs and would love to know if that's misguided here. 🙂

JoshuaKGoldberg · 2024-03-01T19:30:56Z

lib/cli/options.js

+    const configObject = config ? loadConfig(config) : {};
+
+    // Set _configPath for use by parse()
+    configObject._configPath = config;


[Refactor] I'm not a big fan of setting "magic" properties that are only used by specific other places. They get hard to keep track of and can accidentally end up being exposed to users. Which is what's happening here: loadRc is a @public function and thus part of the public API. https://mochajs.org/api/module-lib_cli#.loadRc

I think the root need for this code change is that the config value also needs to be available in parse(), right? If so: how about extracting the contents of this if into a private shared function, then utilizing that function in two places?

if (args.config !== false) { return myNewFancyFunction(args).configObject; }

(roughly that, but maybe with more clear naming?)

JoshuaKGoldberg · 2024-03-01T19:33:03Z

lib/cli/options.js

[Question] #4980 is also meant to fix extends, and has less code. It looks like #4980 uses the higher-level yargs/yargs -> yargs() instead of this PR's yargs/helpers -> applyExtends. I think it's normally cleaner to use higher-level functions when possible. They tend to do more of the work for consumers - as seen in #4980. Is there an advantage to applyExtends?

👋 @mf-pherlihy was hoping to get your thoughts here. What do you think?

JoshuaKGoldberg · 2024-03-01T19:33:34Z

test/integration/options.spec.js

[Praise] Really clean test, nice 🙂

JoshuaKGoldberg · 2024-03-01T19:35:02Z

test/integration/options.spec.js

+var loadOptions = require('../../lib/cli/options').loadOptions;
+
+describe('options', function() {
+  it('Should support extended options', function() {


[Style] Nit: other tests in this repo generally lower-case the 's'. For consistency...

Suggested change

it('Should support extended options', function() {

it('should support extended options', function() {

JoshuaKGoldberg · 2024-03-01T19:36:59Z

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

JoshuaKGoldberg · 2024-07-02T16:51:13Z

👋 ping @mf-pherlihy, do you still have time for this?

JoshuaKGoldberg · 2024-08-06T15:14:27Z

Closing out as it's been a while since PR activity. If anybody wants to take this over, please do - and post a co-author attribution if you take code from this PR. Cheers! 🤎

mf-pherlihy mentioned this pull request Aug 13, 2020

🐛 Bug: Configurations don't seem to support 'extends' #3916

Open

boneskull reviewed Aug 18, 2020

View reviewed changes

boneskull added type: bug a defect, confirmed by a maintainer semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Aug 18, 2020

outsideris reviewed Aug 22, 2020

View reviewed changes

mf-pherlihy force-pushed the extends-fix branch from a957e8f to 105d1aa Compare January 17, 2021 19:00

mf-pherlihy and others added 2 commits February 13, 2021 11:15

Fixed a bug where configurations did not actually support extends

e49ed30

require public helper

f07d427

juergba force-pushed the extends-fix branch from 105d1aa to f07d427 Compare February 13, 2021 10:40

juergba suggested changes Feb 14, 2021

View reviewed changes

fryorcraken mentioned this pull request Feb 10, 2023

chore: remove usage of jsdom waku-org/js-waku#1166

Merged

manuth added a commit to manuth/mocha that referenced this pull request Apr 24, 2023

test(cli): add tests borrowed from mochajs#4407

72273cb

This was referenced Apr 24, 2023

🐛 Bug: Some Settings Cannot Be Inherited (extends keyword) #4979

Closed

fix: extends not working for some settings #4980

Open

JoshuaKGoldberg requested changes Mar 1, 2024

View reviewed changes

JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Mar 1, 2024

JoshuaKGoldberg changed the title ~~Fixed a bug where configurations did not actually support extends~~ fix: fully support 'extends' for configurations Mar 1, 2024

JoshuaKGoldberg added semver-major implementation requires increase of "major" version number; "breaking changes" and removed semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Mar 1, 2024

JoshuaKGoldberg added the stale this has been inactive for a while... label Jul 2, 2024

JoshuaKGoldberg closed this Aug 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fully support 'extends' for configurations #4407

fix: fully support 'extends' for configurations #4407

mf-pherlihy commented Aug 13, 2020

jsf-clabot commented Aug 13, 2020 •

edited

Loading

boneskull left a comment

outsideris left a comment

boneskull commented Aug 25, 2020

bcoe commented Aug 28, 2020

boneskull commented Aug 28, 2020

boneskull commented Aug 28, 2020

mf-pherlihy commented Aug 31, 2020 •

edited

Loading

boneskull commented Sep 9, 2020

boneskull commented Nov 2, 2020

bcoe commented Nov 3, 2020

mf-pherlihy commented Jan 17, 2021

coveralls commented Jan 17, 2021

bcoe commented Jan 27, 2021

juergba commented Jan 29, 2021

bcoe commented Jan 29, 2021

juergba commented Jan 29, 2021

juergba commented Feb 13, 2021 •

edited

Loading

juergba commented Feb 13, 2021 •

edited

Loading

juergba left a comment

mf-pherlihy commented Feb 23, 2021

bcoe commented Mar 12, 2021

SmashingQuasar commented May 8, 2023

JoshuaKGoldberg commented Jan 18, 2024

JoshuaKGoldberg left a comment

JoshuaKGoldberg Mar 1, 2024

JoshuaKGoldberg Mar 1, 2024

JoshuaKGoldberg Jul 2, 2024

JoshuaKGoldberg Mar 1, 2024

JoshuaKGoldberg Mar 1, 2024

JoshuaKGoldberg commented Mar 1, 2024

JoshuaKGoldberg commented Jul 2, 2024

JoshuaKGoldberg commented Aug 6, 2024

	it('Should support extended options', function() {
	it('should support extended options', function() {

fix: fully support 'extends' for configurations #4407

fix: fully support 'extends' for configurations #4407

Conversation

mf-pherlihy commented Aug 13, 2020

Requirements

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

Applicable issues

jsf-clabot commented Aug 13, 2020 • edited Loading

boneskull left a comment

Choose a reason for hiding this comment

outsideris left a comment

Choose a reason for hiding this comment

boneskull commented Aug 25, 2020

bcoe commented Aug 28, 2020

boneskull commented Aug 28, 2020

boneskull commented Aug 28, 2020

mf-pherlihy commented Aug 31, 2020 • edited Loading

boneskull commented Sep 9, 2020

boneskull commented Nov 2, 2020

bcoe commented Nov 3, 2020

mf-pherlihy commented Jan 17, 2021

coveralls commented Jan 17, 2021

bcoe commented Jan 27, 2021

juergba commented Jan 29, 2021

bcoe commented Jan 29, 2021

juergba commented Jan 29, 2021

juergba commented Feb 13, 2021 • edited Loading

juergba commented Feb 13, 2021 • edited Loading

juergba left a comment

Choose a reason for hiding this comment

mf-pherlihy commented Feb 23, 2021

bcoe commented Mar 12, 2021

SmashingQuasar commented May 8, 2023

JoshuaKGoldberg commented Jan 18, 2024

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 1, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 1, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Jul 2, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 1, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 1, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg commented Mar 1, 2024

JoshuaKGoldberg commented Jul 2, 2024

JoshuaKGoldberg commented Aug 6, 2024

jsf-clabot commented Aug 13, 2020 •

edited

Loading

mf-pherlihy commented Aug 31, 2020 •

edited

Loading

juergba commented Feb 13, 2021 •

edited

Loading

juergba commented Feb 13, 2021 •

edited

Loading