Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Enhancement and bug fix to address an npm issue #8

Closed
wants to merge 4 commits into from

2 participants

Martin Cooper Isaac Z. Schlueter
Martin Cooper

(I'd intended for this to be 2 patches, but GitHub decided it has to be one. Sorry about that.)

Allow for one ConfigChain to extend another

This is needed in order to fix npm/npm#3121.

HOLD FOR NOW I'm working on a better, less hacky, approach.

Add the ability to create a new ConfigChain instance from an existing one. The npm use case (see issue above) is to prepend the publishConfig to the existing config, such that any new values are transparently accessible. The bug that needs to be fixed is that the sources were being lost, so any named lookups fail.

Since objects only have a single prototype chain, it's not feasible to clone the whole config chain. However, we can clone the list, the sources, and the root, so that the new instance retains all of the functionality of the original. If a new object is added to the front of the config chain using 'unshift' (which is what npm does), the values in the new object are available only via the new config chain instance. However since the 'add' functions append to the prototype chain, they will add to both the old and the new config chain instances.

Bug fix for retrieving named config values

There's a bug in the get() function when retrieving a value from a named config, when that named config is either the only or the last config in the chain. For example, the following code throws an exception:

var cc = new CC()
cc.add({foo:'flooby'}, 'named_config')
  .on('load', function (cc) {
    cc.get('foo', 'named_config')
  })

What's happening is that proto-list sets the __proto__ value of the config data to its root, but for the first guy in, root is null, at which point the data object is no longer a proper Object, and no longer has a hasOwnProperty() function. So when get() tries to call hasOwnProperty(), it throws because the function doesn't exist.

I've fixed this by always using Object.hasOwnProperty, instead of trying to call it as a function on the data object. I've also added a bunch of unit tests for named configs.

Isaac Z. Schlueter
Collaborator

@mfncooper So, you said to "hold for now"... Should I review this? Is it good? Thanks.

Martin Cooper

No, I think we should just delete this, and I'll replace it later (hopefully soon). While these changes appear to solve the problem at the config-chain level, and all of the tests, new and old, pass, it turned out that the npmconf use cases break quite badly (for legitimate reasons). The solution is a set of slightly deeper changes to config-chain derivation. I started on those changes, but then got pulled away onto other things. I'll try to get back to this soon ...

Isaac Z. Schlueter
Collaborator

Ok, thanks for the update. npm is mostly working for now, so no rush.

Isaac Z. Schlueter isaacs closed this April 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
15  index.js
@@ -83,12 +83,23 @@ var env = exports.env = function (prefix, env) {
83 83
 }
84 84
 
85 85
 exports.ConfigChain = ConfigChain
86  
-function ConfigChain () {
  86
+function ConfigChain (base) {
87 87
   EE.apply(this)
88 88
   ProtoList.apply(this, arguments)
89 89
   this._awaiting = 0
90 90
   this._saving = 0
91 91
   this.sources = {}
  92
+  if (base) {
  93
+    if (base instanceof ConfigChain) {
  94
+      this.list = base.list.slice(0)
  95
+      this.root = base.root
  96
+      for (var k in base.sources) {
  97
+        this.sources[k] = base.sources[k]
  98
+      }
  99
+    } else {
  100
+      this.root = base
  101
+    }
  102
+  }
92 103
 }
93 104
 
94 105
 // multi-inheritance-ish
@@ -141,7 +152,7 @@ ConfigChain.prototype.get = function (key, where) {
141 152
   if (where) {
142 153
     where = this.sources[where]
143 154
     if (where) where = where.data
144  
-    if (where && where.hasOwnProperty(key)) return where[key]
  155
+    if (where && Object.hasOwnProperty.call(where, key)) return where[key]
145 156
     return undefined
146 157
   }
147 158
   return this.list[0][key]
39  test/extend.js
... ...
@@ -0,0 +1,39 @@
  1
+var test = require('tap').test
  2
+var CC = require('../index.js').ConfigChain
  3
+
  4
+test('test chain extension', function (t) {
  5
+  var cc = new CC()
  6
+
  7
+  cc.add({foo: 'foo_noname'})
  8
+    .add({foo: 'foo_named', bar: 'bar_named'}, 'named')
  9
+    .on('load', function (cc) {
  10
+
  11
+      t.same(cc.get('foo'), 'foo_noname')
  12
+      t.same(cc.get('foo', 'named'), 'foo_named')
  13
+      t.same(cc.get('bar'), 'bar_named')
  14
+      t.same(cc.get('bar', 'named'), 'bar_named')
  15
+
  16
+      var cc2 = new CC(cc)
  17
+
  18
+      t.same(cc2.get('foo'), 'foo_noname')
  19
+      t.same(cc2.get('foo', 'named'), 'foo_named')
  20
+      t.same(cc2.get('bar'), 'bar_named')
  21
+      t.same(cc2.get('bar', 'named'), 'bar_named')
  22
+
  23
+      // Add uses push, so adds to both base and extended chains ...
  24
+      cc2.add({baz: 'baz_named2'}, 'named2')
  25
+      t.same(cc.get('baz'), 'baz_named2')
  26
+      t.same(cc2.get('baz'), 'baz_named2')
  27
+      // ... but the new named config only exists in cc2
  28
+      t.same(cc.get('baz', 'named2'), undefined)
  29
+      t.same(cc2.get('baz', 'named2'), 'baz_named2')
  30
+
  31
+      // Using unshift will only affect cc2, with no effect on cc
  32
+      cc2.unshift({qux: 'qux_noname'})
  33
+      t.same(cc.get('qux'), undefined)
  34
+      t.same(cc2.get('qux'), 'qux_noname')
  35
+
  36
+      t.pass('ok')
  37
+      t.end()
  38
+    })
  39
+})
34  test/named.js
... ...
@@ -0,0 +1,34 @@
  1
+var test = require('tap').test
  2
+var CC = require('../index.js').ConfigChain
  3
+
  4
+test('test named configs', function (t) {
  5
+  var cc = new CC()
  6
+
  7
+  cc.add({foo: 'foo_cfg1'}, 'cfg1')
  8
+    .add({foo: 'foo_noname', bar: 'bar_noname'})
  9
+    .add({foo: 'foo_cfg2', baz: 'baz_cfg2'}, 'cfg2')
  10
+    .on('load', function (cc) {
  11
+
  12
+      t.same(cc.get('foo'), 'foo_cfg1')
  13
+      t.same(cc.get('foo', 'cfg1'), 'foo_cfg1')
  14
+      t.same(cc.get('foo', 'cfg2'), 'foo_cfg2')
  15
+      t.same(cc.get('foo', 'bad'), undefined)
  16
+
  17
+      t.same(cc.get('bar'), 'bar_noname')
  18
+      t.same(cc.get('baz'), 'baz_cfg2')
  19
+      t.same(cc.get('baz', 'cfg2'), 'baz_cfg2')
  20
+
  21
+      cc.set('baz', 'baz_new', 'cfg1')
  22
+      t.same(cc.get('baz'), 'baz_new')
  23
+      t.same(cc.get('baz', 'cfg1'), 'baz_new')
  24
+      t.same(cc.get('baz', 'cfg2'), 'baz_cfg2')
  25
+
  26
+      cc.set('bar', 'bar_new')
  27
+      t.same(cc.get('bar'), 'bar_new')
  28
+      t.same(cc.get('bar', 'cfg1'), 'bar_new')
  29
+      t.same(cc.get('bar', 'cfg2'), undefined)
  30
+
  31
+      t.pass('ok')
  32
+      t.end()
  33
+    })
  34
+})
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.