Allow provided config object to extend other configs #779

rcoy-v · 2017-02-10T15:45:53Z

No description provided.

nexdrew

With a couple tweaks, we could make use of this apply-extends logic for the pkgConf(key, path) method as well.

This would allow, for example, a project to define an "nyc" stanza in their package.json file that extends from another "nyc" stanza in a different package.json file.

node_modules/my-lib/package.json:

{
  "nyc": {
    "lines": 95,
    "statements": 95,
    "functions": 95,
    "branches": 95,
    "reporter": [
        "cobertura",
        "lcov"
    ],
    "all": false,
    "check-coverage": true,
    "report-dir": "./coverage",
    "cache": false
  }
}

package.json:

{
  "nyc": {
    "extends": "node_modules/my-lib/package.json",
    "lines": 80
  }
}

Just a suggestion, worked for me locally with the suggested tweaks below, but isn't necessarily a requirement.

Besides that, just one other question and comment. Thanks!

nexdrew · 2017-02-12T05:39:57Z

lib/apply-extends.js

+var path = require('path')
+var assign = require('./assign')
+
+function applyExtends (config, cwd) {


[Tweak 1/3] Add optional third argument here, such as:

function applyExtends (config, cwd, subkey) {

nexdrew · 2017-02-12T05:41:11Z

lib/apply-extends.js

+    delete config.extends
+
+    defaultConfig = JSON.parse(fs.readFileSync(pathToDefault, 'utf8'))
+    defaultConfig = applyExtends(defaultConfig, path.dirname(pathToDefault))


[Tweak 2/3] Check for and use the optional third argument here:

if (subkey) defaultConfig = defaultConfig[subkey] || {} defaultConfig = applyExtends(defaultConfig, path.dirname(pathToDefault), subkey)

nexdrew · 2017-02-12T05:43:27Z

yargs.js

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


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

nexdrew · 2017-02-12T05:55:02Z

lib/apply-extends.js

+    delete config.extends
+
+    defaultConfig = JSON.parse(fs.readFileSync(pathToDefault, 'utf8'))
+    defaultConfig = applyExtends(defaultConfig, path.dirname(pathToDefault))


[Question] I wonder if we should guard against circular references? It might be unlikely but would overflow the stack. Perhaps this submodule should keep track of the paths it has already loaded?

nexdrew · 2017-02-12T06:01:22Z

lib/apply-extends.js

+  var defaultConfig = {}
+
+  if (config.hasOwnProperty('extends')) {
+    var pathToDefault = path.join(cwd, config.extends)


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

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.

bcoe · 2017-02-18T07:23:33Z

@rcoy-v I'm going to try to put a few hours in against yargs tomorrow, and would love to work on landing this.

Do you have any time to address @nexdrew's suggestions, I could also work on making a few edits and landing this myself -- your call.

rcoy-v · 2017-02-18T13:03:46Z

I might be able to pick this up again tomorrow, but I can't promise that. I'm fine if you want to make edits to this pull request.

bcoe · 2017-02-18T17:19:46Z

@rcoy-v awesome, I'm really excited about this feature.

JaKXz

At a glance this is looking good :) but looks like it needs a rebase, since I believe there are a few files already in the 7.x branch

JaKXz · 2017-02-19T07:31:55Z

test/yargs.js

+
+        argv.a.should.equal(1)
+        argv.b.should.equal(22)
+        argv.z.should.equal(15)


🔥 this is great.

my only nitpick is about the indenting on line 1162.

bcoe · 2017-02-20T18:35:55Z

@rcoy-v @JaKXz I'm at Hack Illinois this coming weekend; I was thinking getting yargs@7 out over the weekend would be a great task for my team -- tldr; expect to see a bunch of these tickets land on the weekend.

nexdrew

This is great work, thanks Ryan!

Only one little change requested, which I'll attempt to submit a PR for against your branch.

A couple other things we might want to consider as well (not necessarily required right now):

Perhaps we should document the "extends" capability in the README (or wherever we're putting docs these days)
Should we be concerned with OS-specific paths in "extends"? I'm guessing no at this point, but perhaps it's something we could accommodate for when loading files from "extends"?

nexdrew · 2017-02-20T21:23:03Z

test/yargs.js

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


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!

Done with your help in 5353a2b.

bcoe · 2017-02-20T21:42:06Z

@nexdrew is this the new argument validation, I think we might find a few edge-cases where we need to loosen the rules a bit.

nexdrew · 2017-02-20T21:44:19Z

@bcoe

is this the new argument validation?

Negative. This error is thrown by the builtin Node path module. The argsert definition for .pkgConf() looks correct to me:

argsert('<string> [string]', [key, path], arguments.length)

The 2nd string is optional.

JaKXz · 2017-02-20T22:21:54Z

@nexdrew won't arguments.length give you a number [which doesn't match the type of optional string]?

nexdrew · 2017-02-20T23:44:27Z

@JaKXz As I understand it, the 3rd argument to argsert() should be the number of arguments given. The actual types are checked via the 2nd argument, which is an array representing the actual argument values.

bcoe · 2017-02-20T23:49:18Z

@JaKXz, @nexdrew is correct -- the use of argsert in yargs is dead-simple, based on various performance bike-shedding discussions.

nexdrew · 2017-02-20T23:50:25Z

@rcoy-v See this PR against your branch to add the changes I requested. I think that will put this over the finish line. Let me know what you think. Thanks!

- handle absolute paths - add note about "extends" to config and pkgConf docs - use cwd when no path given to pkgConf(key) method

rcoy-v · 2017-02-23T00:13:26Z

lib/apply-extends.js

+}
+
+function getPathToDefaultConfig (cwd, pathToExtend) {
+  return path.isAbsolute(pathToExtend) ? pathToExtend : path.join(cwd, pathToExtend)


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.

Would path.resolve(pathToExtend) work?

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.

rcoy-v · 2017-02-23T00:15:18Z

test/yargs.js

+  })
+
+  afterEach(function () {
+    delete require.cache[require.resolve('../')]


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.

BREAKING CHANGE: `extends` key in config file is now used for extending other config files

bcoe · 2017-02-27T00:28:32Z

@rcoy-v, @JaKXz, @nexdrew, phew! thanks for all the hard-work on this feature, laded this weekend as part of our big push towards yargs 7.x.

I would love some help testing, before we promote this to a wider audience:

npm cache clear; npm i yargs@next

@rcoy-v this means we can finally upstream this functionality to nyc 😄

rcoy-v mentioned this pull request Feb 10, 2017

Allow configuration to extend other config files #775

Closed

nexdrew reviewed Feb 12, 2017

View reviewed changes

bcoe force-pushed the 7.x branch from cef98c8 to 3d0490c Compare February 18, 2017 18:15

bcoe changed the title ~~Allow provided config object to extend other configs~~ [WIP] Allow provided config object to extend other configs Feb 18, 2017

JaKXz reviewed Feb 19, 2017

View reviewed changes

rcoy-v force-pushed the config-extend branch from e07fac6 to bf46a79 Compare February 19, 2017 18:03

rcoy-v changed the title ~~[WIP] Allow provided config object to extend other configs~~ Allow provided config object to extend other configs Feb 19, 2017

Allow provided config object to extend other configs

10851a9

rcoy-v force-pushed the config-extend branch from 08ddd1b to 10851a9 Compare February 20, 2017 02:35

nexdrew requested changes Feb 20, 2017

View reviewed changes

nexdrew mentioned this pull request Feb 20, 2017

Use cwd when no path given to pkgConf(key) method rcoy-v/yargs#1

Closed

Address latest review comments w/ help from @nexdrew

5353a2b

- handle absolute paths - add note about "extends" to config and pkgConf docs - use cwd when no path given to pkgConf(key) method

rcoy-v commented Feb 23, 2017

View reviewed changes

bcoe added the hackillinois label Feb 25, 2017

stevezhu merged commit e2a2133 into yargs:7.x Feb 25, 2017

stevezhu mentioned this pull request Feb 25, 2017

fix: extending config files breaks on node 0.10 #797

Merged

bcoe pushed a commit that referenced this pull request Feb 26, 2017

feat: allow provided config object to extend other configs (#779)

3280dd0

BREAKING CHANGE: `extends` key in config file is now used for extending other config files

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow provided config object to extend other configs #779

Allow provided config object to extend other configs #779

rcoy-v commented Feb 10, 2017

nexdrew left a comment •

edited

nexdrew Feb 12, 2017

nexdrew Feb 12, 2017

nexdrew Feb 12, 2017

rcoy-v Feb 19, 2017

nexdrew Feb 12, 2017

rcoy-v Feb 19, 2017

nexdrew Feb 12, 2017

rcoy-v Feb 19, 2017

bcoe commented Feb 18, 2017

rcoy-v commented Feb 18, 2017

bcoe commented Feb 18, 2017

JaKXz left a comment

JaKXz Feb 19, 2017

rcoy-v Feb 19, 2017

bcoe commented Feb 20, 2017

nexdrew left a comment

nexdrew Feb 20, 2017

rcoy-v Feb 23, 2017

bcoe commented Feb 20, 2017

nexdrew commented Feb 20, 2017

JaKXz commented Feb 20, 2017

nexdrew commented Feb 20, 2017

bcoe commented Feb 20, 2017

nexdrew commented Feb 20, 2017

rcoy-v Feb 23, 2017

scriptdaemon Feb 23, 2017

nexdrew Feb 24, 2017

rcoy-v Feb 23, 2017 •

edited

bcoe commented Feb 27, 2017

Allow provided config object to extend other configs #779

Allow provided config object to extend other configs #779

Conversation

rcoy-v commented Feb 10, 2017

nexdrew left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe commented Feb 18, 2017

rcoy-v commented Feb 18, 2017

bcoe commented Feb 18, 2017

JaKXz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe commented Feb 20, 2017

nexdrew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe commented Feb 20, 2017

nexdrew commented Feb 20, 2017

JaKXz commented Feb 20, 2017

nexdrew commented Feb 20, 2017

bcoe commented Feb 20, 2017

nexdrew commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcoy-v Feb 23, 2017 • edited

Choose a reason for hiding this comment

bcoe commented Feb 27, 2017

nexdrew left a comment •

edited

rcoy-v Feb 23, 2017 •

edited