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 all commits
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
4 changes: 4 additions & 0 deletions README.md
Expand Up @@ -980,6 +980,8 @@ $ node test.js
'$0': 'test.js' }
```

Note that a configuration object may extend from a JSON file using the `"extends"` property. When doing so, the `"extends"` value should be a path (relative or absolute) to the extended JSON file.

<a name="conflicts"></a>.conflicts(x, y)
----------------------------------------------

Expand Down Expand Up @@ -1649,6 +1651,8 @@ as a configuration object.
`cwd` can optionally be provided, the package.json will be read
from this location.

Note that a configuration stanza in package.json may extend from an identically keyed stanza in another package.json file using the `"extends"` property. When doing so, the `"extends"` value should be a path (relative or absolute) to the extended package.json file.

.recommendCommands()
---------------------------

Expand Down
41 changes: 41 additions & 0 deletions lib/apply-extends.js
@@ -0,0 +1,41 @@
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 getPathToDefaultConfig (cwd, pathToExtend) {
return path.isAbsolute(pathToExtend) ? pathToExtend : path.join(cwd, pathToExtend)
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 use path.isAbsolute here. According to node docs this was introduced in v0.11.2. I'm not sure where the topic of v0.10 support has landed. Maybe it depends on outcome of latest discussion in #742. If path.isAbsolute cannot be used, I will figure something else out.

Choose a reason for hiding this comment

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

Would path.resolve(pathToExtend) work?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I believe path.resolve() would work here. I actually thought that path.join() worked the same way, which is why I said "relative or absolute" in the docs, but apparently it doesn't.

}

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

if (config.hasOwnProperty('extends')) {
var pathToDefault = getPathToDefaultConfig(cwd, config.extends)

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"
}
}
61 changes: 58 additions & 3 deletions test/yargs.js
@@ -1,16 +1,21 @@
/* global context, describe, it, beforeEach */
/* global context, describe, it, beforeEach, afterEach */

var expect = require('chai').expect
var fs = require('fs')
var path = require('path')
var checkOutput = require('./helpers/utils').checkOutput
var yargs = require('../')
var yargs
var YError = require('../lib/yerror')

require('chai').should()

describe('yargs dsl tests', function () {
beforeEach(function () {
yargs.reset()
yargs = require('../')
})

afterEach(function () {
delete require.cache[require.resolve('../')]
Copy link
Member Author

@rcoy-v rcoy-v Feb 23, 2017

Choose a reason for hiding this comment

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

yargs.reset() does not fully reset the yargs instance. Configs were bleeding into other tests, affecting assertions. So I made it that each test has a truly fresh yargs.

})

it('should use bin name for $0, eliminating path', function () {
Expand Down Expand Up @@ -1155,6 +1160,42 @@ 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)
})

it('handles aboslute paths', function () {
var absolutePath = path.join(process.cwd(), 'test', 'fixtures', 'extends', 'config_1.json')

var argv = yargs
.config({
a: 2,
extends: absolutePath
})
.argv

argv.a.should.equal(2)
argv.b.should.equal(22)
argv.z.should.equal(15)
})
})
})

describe('normalize', function () {
Expand Down Expand Up @@ -1411,6 +1452,20 @@ 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')
})

it('should apply extended configurations from cwd when no path is given', function () {
var argv = yargs('', 'test/fixtures/extends/packageA').pkgConf('foo').argv

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

describe('skipValidation', function () {
Expand Down
10 changes: 8 additions & 2 deletions 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 @@ -469,11 +472,14 @@ function Yargs (processArgs, cwd, parentRequire) {
self.pkgConf = function (key, path) {
argsert('<string> [string]', [key, path], arguments.length)
var conf = null
var obj = pkgUp(path)
// prefer cwd to require-main-filename in this method
// since we're looking for e.g. "nyc" config in nyc consumer
// rather than "yargs" config in nyc (where nyc is the main filename)
var obj = pkgUp(path || cwd)

// 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 || cwd, key)
options.configObjects = (options.configObjects || []).concat(conf)
}

Expand Down