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

Add support for throwing if failed #165

Merged
merged 1 commit into from
Dec 23, 2016

Conversation

IOAyman
Copy link
Contributor

@IOAyman IOAyman commented Dec 20, 2016

If verbose: 'throw' is passed in options, an Error is thrown instead of console.erroring to the stdout

@coveralls
Copy link

coveralls commented Dec 20, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 182505f on IOAyman:patch-option-throw into 8117fc1 on motdotla:master.

@maxbeatty
Copy link
Contributor

I think a better alternative would be to return the error instead of false so consumers can handle the error as they please (throw, report, ignore, etc.)

@IOAyman
Copy link
Contributor Author

IOAyman commented Dec 20, 2016

Yeah, it's better than returning false.
How about supporting both approaches? Returning the Error by default, but give the ability to throw if explicitly required via the options.

@maxbeatty
Copy link
Contributor

Please correct me if I'm wrong. The motivation to add an option to throw the error is to control how the error is handled.

Instead of adding options, perhaps it's better to remove one and change what's returned from config. It keeps the footprint and complexity of this module small while allowing the most flexibility for consumers. Taking a page from Hapi's Joi module, we could return an object with an error key that is either null or whatever error occurred and a parsed key with the result of parsing the .env file.

import { config } from 'dotenv'
import variableExpansion from 'dotenv-expand'

env = config()

if (env.error) {
  // handle error as you like
  throw env.error
}

variableExpansion(env.parsed)

This is a low-impact path forward for anyone upgrading major versions. It allows people to continue ignoring errors that are most likely due to a missing file in environments like CI.

  • remove verbose option
  • change return value of config
  • update README
  • update CHANGELOG

@coveralls
Copy link

coveralls commented Dec 21, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 25ffb08 on IOAyman:patch-option-throw into f783543 on motdotla:master.

Copy link
Contributor

@maxbeatty maxbeatty left a comment

Choose a reason for hiding this comment

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

looks good. will wait for more feedback and review before merging

@jcblw
Copy link
Collaborator

jcblw commented Dec 21, 2016

This looks good my only concern is with the the return object change and external plugins that do not have dotenv as a dep, eg. dotenv-expand, which has no control if v2 or v3 of dotenv is used.

I do not think there is many of these plugins out there, but I do think it would be a good idea, if this is merged, that we make either PRs or issues to update to support both versions. So that way on release we are not breaking those plugins functionality.

@maxbeatty
Copy link
Contributor

other modules should specify peer dependencies to indicate which versions of dotenv they are compatible with

@motdotla
Copy link
Owner

other modules should specify peer dependencies to indicate which versions of dotenv they are compatible with

I agree.

👍 for this PR

@maxbeatty maxbeatty merged commit 2d84b5a into motdotla:master Dec 23, 2016
@IOAyman IOAyman deleted the patch-option-throw branch December 23, 2016 18:25
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.

5 participants