Skip to content

Fixed core bug with process.env variables not being used, closes #12#226

Closed
niftylettuce wants to merge 4 commits into
motdotla:masterfrom
niftylettuce:master
Closed

Fixed core bug with process.env variables not being used, closes #12#226
niftylettuce wants to merge 4 commits into
motdotla:masterfrom
niftylettuce:master

Conversation

@niftylettuce

Copy link
Copy Markdown

No description provided.

@niftylettuce

Copy link
Copy Markdown
Author

tests are failing and look absolutely horrid to try to fix, here's some output:

Failed tests:

  4) dotenv config reads path with encoding, parsing output to process.env:

      actual expected

      01

      expected 0 to equal 1

      at Assertion.fail (/Users/nexus/Public/dotenv/node_modules/should/cjs/should.js:205:17)
      at Assertion.value (/Users/nexus/Public/dotenv/node_modules/should/cjs/should.js:277:19)
      at /Users/nexus/Public/dotenv/test/main.js:56:34
      at Immediate.setImmediate (/Users/nexus/Public/dotenv/node_modules/lab/lib/runner.js:647:36)
      at runCallback (timers.js:781:20)
      at tryOnImmediate (timers.js:743:5)
      at processImmediate [as _immediateCallback] (timers.js:714:5)

  5) dotenv config makes load a synonym of config:

      actual expected

      01

      expected 0 to equal 1

      at Assertion.fail (/Users/nexus/Public/dotenv/node_modules/should/cjs/should.js:205:17)
      at Assertion.value (/Users/nexus/Public/dotenv/node_modules/should/cjs/should.js:277:19)
      at /Users/nexus/Public/dotenv/test/main.js:64:34
      at Immediate.setImmediate (/Users/nexus/Public/dotenv/node_modules/lab/lib/runner.js:647:36)
      at runCallback (timers.js:781:20)
      at tryOnImmediate (timers.js:743:5)
      at processImmediate [as _immediateCallback] (timers.js:714:5)

  8) dotenv config returns parsed object:

      actual expected

      {
        "test": """val"
      }

      expected Object { test: '' } to equal Object { test: 'val' } (at test, A has '' and B has 'val')

      at Assertion.fail (/Users/nexus/Public/dotenv/node_modules/should/cjs/should.js:205:17)
      at Assertion.value (/Users/nexus/Public/dotenv/node_modules/should/cjs/should.js:277:19)
      at /Users/nexus/Public/dotenv/test/main.js:90:25
      at Immediate.setImmediate (/Users/nexus/Public/dotenv/node_modules/lab/lib/runner.js:647:36)
      at runCallback (timers.js:781:20)
      at tryOnImmediate (timers.js:743:5)
      at processImmediate [as _immediateCallback] (timers.js:714:5)


3 of 22 tests failed
Test duration: 329 ms
The following leaks were detected:WebAssembly

error Command failed with exit code 1.

@niftylettuce niftylettuce mentioned this pull request Sep 11, 2017
15 tasks
Comment thread lib/main.js Outdated
if (options.encoding) {
encoding = options.encoding
}
if (!options.assign) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This overrides you default of true if someone does not pass anything.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@jcblw I've fixed this in my latest push <c350650

@niftylettuce

Copy link
Copy Markdown
Author

Any word here? This is otherwise broken...

@jcblw

jcblw commented Sep 22, 2017

Copy link
Copy Markdown
Contributor

@niftylettuce can you please into resolving the test first before we look into merging.

I need to go back and look into the other conversations about this same functionality. I don't think I had any objections but @maxbeatty I think had some input at the time. @motdotla any comments?

@maxbeatty

Copy link
Copy Markdown
Contributor

Absolutely do not want this. It can be very dangerous to override existing variables. Please see previous comments on this topic for further explanation.

@maxbeatty maxbeatty closed this Sep 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants