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

Allow provided config object to extend other configs #779

Merged
merged 2 commits into from Feb 25, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions .editorconfig
@@ -0,0 +1,7 @@
root = true

[*.js]
end_of_line = lf
indent_style = space
indent_size = 2
insert_final_newline = true
37 changes: 37 additions & 0 deletions lib/apply-extends.js
@@ -0,0 +1,37 @@
var fs = require('fs')
var path = require('path')
var assign = require('./assign')
var YError = require('./yerror')

var previouslyVisitedConfigs = []

function checkForCircularExtends (path) {
if (previouslyVisitedConfigs.indexOf(path) > -1) {
throw new YError("Circular extended configurations: '" + path + "'.")
}
}

function applyExtends (config, cwd, subKey) {
var defaultConfig = {}

if (config.hasOwnProperty('extends')) {
var pathToDefault = path.join(cwd, config.extends)
Copy link
Member

Choose a reason for hiding this comment

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

[Comment] I like @JaKXz's idea of supporting modules by name as well (e.g. "extends": "my-lib" instead of just "extends": "node_modules/my-lib/some-file"), but since yargs isn't always loading the original "some-file" and doesn't know its path, I like your general approach as the next best thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel yargs is too general of a tool to make assumptions for shorthand extends like my-lib. For more context, my use case in nyc probably won't even extend a root .nycrc in another package. Rather, it will extend something like my-lib/configs/.nycrc.


checkForCircularExtends(pathToDefault)

previouslyVisitedConfigs.push(pathToDefault)
delete config.extends

defaultConfig = JSON.parse(fs.readFileSync(pathToDefault, 'utf8'))
if (subKey) {
defaultConfig = defaultConfig[subKey] || {}
}
defaultConfig = applyExtends(defaultConfig, path.dirname(pathToDefault), subKey)
}

previouslyVisitedConfigs = []

return assign(defaultConfig, config)
}

module.exports = applyExtends
4 changes: 4 additions & 0 deletions test/fixtures/extends/circular_1.json
@@ -0,0 +1,4 @@
{
"a": 44,
"extends": "./circular_2.json"
}
4 changes: 4 additions & 0 deletions test/fixtures/extends/circular_2.json
@@ -0,0 +1,4 @@
{
"b": "any",
"extends": "./circular_1.json"
}
5 changes: 5 additions & 0 deletions test/fixtures/extends/config_1.json
@@ -0,0 +1,5 @@
{
"a": 30,
"b": 22,
"extends": "./config_2.json"
}
3 changes: 3 additions & 0 deletions test/fixtures/extends/config_2.json
@@ -0,0 +1,3 @@
{
"z": 15
}
6 changes: 6 additions & 0 deletions test/fixtures/extends/packageA/package.json
@@ -0,0 +1,6 @@
{
"foo": {
"a": 80,
"extends": "../packageB/package.json"
}
}
6 changes: 6 additions & 0 deletions test/fixtures/extends/packageB/package.json
@@ -0,0 +1,6 @@
{
"foo": {
"a": 90,
"b": "riffiwobbles"
}
}
29 changes: 29 additions & 0 deletions test/yargs.js
Expand Up @@ -5,6 +5,7 @@ var fs = require('fs')
var path = require('path')
var checkOutput = require('./helpers/utils').checkOutput
var yargs = require('../')
var YError = require('../lib/yerror')

require('chai').should()

Expand Down Expand Up @@ -1155,6 +1156,27 @@ describe('yargs dsl tests', function () {
argv.foo.should.equal(1)
argv.bar.should.equal(2)
})

describe('extends', function () {
it('applies default configurations when given config object', function () {
var argv = yargs
.config({
extends: './test/fixtures/extends/config_1.json',
a: 1
})
.argv

argv.a.should.equal(1)
argv.b.should.equal(22)
argv.z.should.equal(15)
Copy link
Member

Choose a reason for hiding this comment

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

🔥 this is great.

my only nitpick is about the indenting on line 1162.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

})

it('protects against circular extended configurations', function () {
expect(function () {
yargs.config({extends: './test/fixtures/extends/circular_1.json'})
}).to.throw(YError)
})
})
})

describe('normalize', function () {
Expand Down Expand Up @@ -1411,6 +1433,13 @@ describe('yargs dsl tests', function () {

argv.foo.should.equal('a')
})

it('should apply default configurations from extended packages', function () {
var argv = yargs().pkgConf('foo', 'test/fixtures/extends/packageA').argv
Copy link
Member

Choose a reason for hiding this comment

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

The path argument to the .pkgConf(key, path) method is optional. When I don't pass a value, I get this error:

path.js:7
    throw new TypeError('Path must be a string. Received ' + inspect(path));
    ^

TypeError: Path must be a string. Received undefined
    at assertPath (path.js:7:11)
    at Object.join (path.js:1211:7)
    at applyExtends (/Users/abg/git/nexdrew/github/yargs/lib/apply-extends.js:18:30)
    at Object.Yargs.self.pkgConf (/Users/abg/git/nexdrew/github/yargs/yargs.js:479:14)
    at Object.<anonymous> (/Users/abg/git/nexdrew/github/demo/pr779/index.js:2:4)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)

Perhaps we should change line 479 of yargs.js to this:

      conf = applyExtends(obj[key], path || cwd, key) // <-- add the `|| cwd` part

and maybe add a test for this scenario as well?

Otherwise, this is looking really good!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done with your help in 5353a2b.


argv.a.should.equal(80)
argv.b.should.equals('riffiwobbles')
})
})

describe('skipValidation', function () {
Expand Down
5 changes: 4 additions & 1 deletion yargs.js
Expand Up @@ -9,6 +9,7 @@ const Validation = require('./lib/validation')
const Y18n = require('y18n')
const objFilter = require('./lib/obj-filter')
const setBlocking = require('set-blocking')
const applyExtends = require('./lib/apply-extends')
const YError = require('./lib/yerror')

var exports = module.exports = Yargs
Expand Down Expand Up @@ -304,6 +305,7 @@ function Yargs (processArgs, cwd, parentRequire) {
argsert('[object|string] [string|function] [function]', [key, msg, parseFn], arguments.length)
// allow a config object to be provided directly.
if (typeof key === 'object') {
key = applyExtends(key, cwd)
Copy link
Member

Choose a reason for hiding this comment

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

[Tweak 3/3] Then we can make use of applyExtends in the pkgConf method as well:

  self.pkgConf = function (key, path) {
    argsert('<string> [string]', [key, path], arguments.length)
    var conf = null
    var obj = pkgUp(path)

    // If an object exists in the key, add it to options.configObjects
    if (obj[key] && typeof obj[key] === 'object') {
      conf = applyExtends(obj[key], cwd, key) // <-- this line changed
      options.configObjects = (options.configObjects || []).concat(conf)
    }

    return self
  }

Copy link
Member Author

Choose a reason for hiding this comment

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

done

options.configObjects = (options.configObjects || []).concat(key)
return self
}
Expand All @@ -319,6 +321,7 @@ function Yargs (processArgs, cwd, parentRequire) {
;(Array.isArray(key) ? key : [key]).forEach(function (k) {
options.config[k] = parseFn || true
})

return self
}

Expand Down Expand Up @@ -473,7 +476,7 @@ function Yargs (processArgs, cwd, parentRequire) {

// If an object exists in the key, add it to options.configObjects
if (obj[key] && typeof obj[key] === 'object') {
conf = obj[key]
conf = applyExtends(obj[key], path, key)
options.configObjects = (options.configObjects || []).concat(conf)
}

Expand Down