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

Command Substitution #48

Closed
wants to merge 2 commits into from
Closed

Conversation

nVitius
Copy link

@nVitius nVitius commented Jan 21, 2021

I added code to this plugin to do command substitution like bash does: $(some_command)

My personal use case was that I wanted to have the version from package.json inside of a env var in my Vue app.
Using this update, you can do something like: APP_VERSION="$(cat package.json | jq '.version')"

I've added a simple test (using echo) and made sure that all other existing tests are still working.
I think this could be a really valuable addition to the plugin.

@Heziode
Copy link

Heziode commented Jan 21, 2021

You should add an option to allow enable/disable this feature, because the execution of command can be dangerous.

@nVitius
Copy link
Author

nVitius commented Jan 21, 2021

@Heziode The easiest solution that occurs to me is adding a prefix to the keys. Something like: EVAL_APP_VERSION

What do you think?

@Heziode
Copy link

Heziode commented Jan 21, 2021

IMHO, it should be done with an option inside the conf object, like this:

var environment = config.ignoreProcessEnv ? {} : process.env

This will also require to update the inde.d.ts (that also have a missing value that wil be fixed with #47):

interface DotenvResult {
error?: Error;
parsed?: {
[name: string]: string;
};
}

@nVitius
Copy link
Author

nVitius commented Jan 21, 2021

My issue with using the config object is that some of the popular bundlers (namely vue-cli and parcel) use dotenv (and dotenv-expand) but don't expose any way to pass configuration options into it.

Also, dotenv itself doesn't seem to officially support custom config values like ignoreProcessEnv
Reference: https://github.com/motdotla/dotenv/blob/72fb66b051280ef5c2cc40ce4962ac4601f7f515/lib/main.js

At least, they aren't included in the type definition.

@Heziode
Copy link

Heziode commented Jan 21, 2021

My issue with using the config object is that some of the popular bundlers (namely vue-cli and parcel) use dotenv (and dotenv-expand) but don't expose any way to pass configuration options into it.

Indeed, I hadn't thought of that. Afterwards, nothing prevents you from adding it to your project.

For implemenation details, etc. I think that @motdotla is more relevant to answer, since it's his lib.

@cryptiklemur
Copy link

Why not use the config value, with a fallback to an env variable itself if not set

DOTENV_EVAL=false or something

@nVitius
Copy link
Author

nVitius commented Mar 1, 2021

@motdotla
Can we get your feedback on this PR please?

@motdotla
Copy link
Owner

@nVitius thank you so much for this contribution 💛

That said, we decided to move it to its own module here: https://github.com/motdotla/dotenv-eval

We should see that module grow in complexity as it handles any edge cases.

@motdotla motdotla closed this Feb 10, 2022
@nVitius
Copy link
Author

nVitius commented Feb 10, 2022

@motdotla a lot of projects use dotenv through an external tool and don't have access to configure dotenv or add new dependencies. I use dotenv a lot in my vue projects and vue-cli handles loading in dotenv. There would be no way for me to leverage command substitution in this case. Have you given any thought on how that might be solved for?

@motdotla
Copy link
Owner

@nVitius very good point.

So does vue-cli already default include dotenv-expand?

@motdotla motdotla reopened this Feb 10, 2022
@motdotla
Copy link
Owner

(a part of me is also starting to think that maybe expand's functionality should move directly into Dotenv's)

@Heziode
Copy link

Heziode commented Feb 11, 2022

(a part of me is also starting to think that maybe expand's functionality should move directly into Dotenv's)

This is exactly what I did, when I rewrote your lib in Ada: https://github.com/Heziode/ada-dotenv

@ccmattr
Copy link

ccmattr commented May 12, 2022

(a part of me is also starting to think that maybe expand's functionality should move directly into Dotenv's)

It would be amazing if dotenv-expand and dotenv-eval could be included by default. Theres no reason to say the package users couldnt limit the functionality through configuration.

@motdotla motdotla closed this Feb 6, 2024
This pull request was closed.
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