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.

Sign up for GitHub

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: fully support 'extends' for configurations #4407

Closed
wants to merge 2 commits into from
Closed

fix: fully support 'extends' for configurations #4407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e49ed30
Fixed a bug where configurations did not actually support extends
mf-pherlihy Aug 13, 2020
f07d427
require public helper
juergba Feb 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions lib/cli/options.js
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
*/

const fs = require('fs');
const path = require('path');
const ansi = require('ansi-colors');
const {cwd} = require('../utils');
const {applyExtends} = require('yargs/helpers');
const yargsParser = require('yargs-parser');
const {types, aliases} = require('./run-option-metadata');
const {ONE_AND_DONE_ARGS} = require('./one-and-dones');
Expand Down Expand Up @@ -124,7 +127,19 @@ const parse = (args = [], defaultValues = {}, ...configObjects) => {

const result = yargsParser.detailed(args, {
configuration,
configObjects,
configObjects: configObjects.map(configObject => {
// '_configPath' gets set by config.loadConfig()
const config = configObject || {};
const configPath =
typeof config._configPath === 'string'
? path.dirname(config._configPath)
: cwd();

// Remove the internal property
delete config._configPath;

return applyExtends(config, configPath, true);
}),
default: defaultValues,
coerce: coerceOpts,
narg: nargOpts,
Expand Down Expand Up @@ -158,7 +173,12 @@ const parse = (args = [], defaultValues = {}, ...configObjects) => {
const loadRc = (args = {}) => {
if (args.config !== false) {
const config = args.config || findConfig();
return config ? loadConfig(config) : {};
const configObject = config ? loadConfig(config) : {};

// Set _configPath for use by parse()
configObject._configPath = config;
Copy link
Member

Choose a reason for hiding this comment

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

[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?)


return configObject;
}
};

Expand Down
6 changes: 6 additions & 0 deletions test/integration/fixtures/config/mocharc-extended/base.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"require": ["foo", "bar"],
"bail": true,
"reporter": "dot",
"slow": 60
}
3 changes: 3 additions & 0 deletions test/integration/fixtures/config/mocharc-extended/extends.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"extends": "./modifiers.json"
}
5 changes: 5 additions & 0 deletions test/integration/fixtures/config/mocharc-extended/modifiers.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"extends": "./base.json",
"reporter": "html",
"slow": 30
}
23 changes: 23 additions & 0 deletions test/integration/options.spec.js
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

[Praise] Really clean test, nice 🙂

Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

var path = require('path');
var loadOptions = require('../../lib/cli/options').loadOptions;

describe('options', function() {
it('Should support extended options', function() {
Copy link
Member

Choose a reason for hiding this comment

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

[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() {

var configDir = path.join(
__dirname,
'fixtures',
'config',
'mocharc-extended'
);
var extended = loadOptions([
'--config',
path.join(configDir, 'extends.json')
]);
expect(extended.require, 'to equal', ['foo', 'bar']);
expect(extended.bail, 'to equal', true);
expect(extended.reporter, 'to equal', 'html');
expect(extended.slow, 'to equal', 30);
});
});
Loading