Nested environment variables with unassigned empty level in between

[jcollado]

I had problems with rc trying to deal with nested environment variables that have a empty level in between such as project_someopt__a and project_someopt__a__b__c (see environment variable added to testing code).

It turns out that the old code was assigning properties to primitive values and that was confusing. To prevent this from happening strict mode has been enabled.

To fix the problem found a new check to make sure the assignment only happens to objects seems to do the job. Note that a primitive value would mean that there has been already an assignment at an upper lever and hence the following ones are expected to be ignored as is indeed checked in the testing code.

[dominictarr]

can you describe what happens when you have a deeply nested envvar in your pr? in particular what the output js object looks like? I can't see what is happening in your tests.

[jcollado]

If I just leave the testing code in this PR, what I get when I run the test cases is:

> rc@1.1.1 test /home/javi/code/git/public/rc
> set -e; node test/test.js; node test/ini.js; node test/nested-env-vars.js

{ option: true, envOption: '42', _: [] }
{ option: false,
envOption: 24,
argv: 
{ remain: [],
    cooked: [ '--no-option', '--envOption', '24' ],
    original: [ '--no-option', '--envOption=24' ] } }
{ option: false,
envOption: '42',
_: [],
configs: [ '/home/javi/code/git/public/rc/.rc0.4336030308622867rc' ],
config: '/home/javi/code/git/public/rc/.rc0.4336030308622867rc' }
hello=true
{"hello":true}
/home/javi/code/git/public/rc/lib/utils.js:67
        cursor[_subkey] = env[k]
                        ^

TypeError: Cannot set property 'c' of undefined
    at _buildSubObj (/home/javi/code/git/public/rc/lib/utils.js:67:27)
    at Array.forEach (native)
    at Object.exports.env (/home/javi/code/git/public/rc/lib/utils.js:57:15)
    at module.exports (/home/javi/code/git/public/rc/index.js:23:16)
    at Object.<anonymous> (/home/javi/code/git/public/rc/test/nested-env-vars.js:21:28)
    at Module._compile (module.js:434:26)
    at Object.Module._extensions..js (module.js:452:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:475:10)
npm ERR! Test failed.  See above for more details.

In the test code there's:

process.env[n+'_someOpt__a'] = 42
process.env[n+'_someOpt__a__b'] = 186
process.env[n+'_someOpt__a__b__c'] = 243

After the first environment variable is assigned we have (removed the someOpt part):

{a: 42}

For the second one, nothing is assigned, but what is really happening is:

{a: 42}.a.b = 186

There's no warning about this unless in strict mode.

Finally for the thind one, this happens:

{a: 42}.a.b.c = 243

However:

{a: 42}.a.b

is undefined and that's why the TypeError above is thrown even without strict mode.

After the PR what happens is that the 42 is detected as not being an object and the subkey traversal stops for both a__b and a__b__c to prevent property assignments to primitive values.

[dominictarr]

Hmm, If we are gonna fix this, lets fix it right. I think it would be much better to check wether a key is being set on a primitive and throwing an error that explained what the problem was. A type error would probably be confusing, and that would mean an issue posted here and I'd get an email.

[jcollado]

Sorry, I think I was editing my comment before you ended reading it. The type error is what happened before the PR. After the PR those assignments don't happen and the environment variables for them are ignored. This is the old behavior, so there's nothing that changes.

[dominictarr]

oh, so all this does is adds strict mode?

[jcollado]

The strict mode is not really needed. Is just there to avoid any corner case in the future. What stops the subkey traversal when a primitive was already assigned at an upper level is the added check: typeof cursor !== 'object'

[dominictarr]

so was there any sort of error that i can reproduce with the current version on npm? how would I get that error, using rc as you would from within an application?

[jcollado]

The error that you can get is the TypeError above if you just use the change in test/nested-env-vars.js in this PR and running the test cases with npm test. I've checked with my colleagues and for some reason this doesn't seem to be reproducible in an OSX machine, but it is in a Linux one.

[jcollado]

Maybe this is helpful. At first I run the test cases with the code in this PR and they pass. Then I comment out the check that has been added and run again the tests to display the type error:

[dominictarr]

Okay so I did a test in the terminal, running rc/index.js as a command, i see that minimist and envvars behave the same way when setting properties on primitives (they both ignore the nested bit)

# envvars
$ foo_bar=1 foo_bar__baz=1 ./index.js foo    
{
  "bar": "1",
  "_": [
    "foo"
  ]
}

# minimist opts
$ ./index.js foo --bar 3 --bar.baz 4
{
  "_": [
    "foo"
  ],
  "bar": 3
}

The error only happens when the application checks foo = require('rc')('foo'); foo.bar.baz and it's undefined. This mightn't be ideal, but at least it's consistent.

Aha, okay it only happens when it's nested 3 deep.

f_a=1 f_a__b=2 f_a__b__c=3 ./index.js f

[jcollado]

The type error above is when the strict mode enabled. Without strict mode, the error is still a TypeError, but the message is no longer Cannot assign to read only property 'b' of 42, but Cannot set property 'c' of undefined.

[dominictarr]

merged into 1.1.2

[jcollado]

Great, thanks!