-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Added support for .env file #3938
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree that scoping environment variables per-project is better than using shell configuration files and I have no problem with supporting .env
in core, I don't think this is the right approach.
- Injecting it in the CLI entry point makes users think Yarn supports
.env
while only the CLI does, anything using the API would not load it. - I believe the right approach would be to use
dotenv
in:Configuration.find
, but returning a new env that's passed down to the functions that expect an env rather than injecting it intoprocess.env
sinceConfiguration.find
shouldn't produce a side effect.- The
setupScriptEnvironment
hook (which injects it into all of your scripts and commands that use an env) with the same mention, not mutatingprocess.env
.
- If we support it in core I don't see any value in not enabling it by default and having it be opt-in.
I agree with you @paul-soporan, what I did is a bit hacky and not a direct fix. I also realised afterwards that it wasn't making What you suggest seems way more solid. I'd have to dig a bit into it as I'm not that familiar with the codebase, but I'd be happy to give it a try. In terms of enabling this feature by default, we'd just have to set |
I've been working on this myself off and on as a way of getting more familiar with the code base in general and have some questions. If the env file is read in with I was looking at implementing this as a plug-in but couldn't work out how to modify the configuration from the plugin without adding a command that would have to be manually invoked. Adding the hook for setupScriptEnviroment seemed straightforward though. Is a plug-in capable of offering this functionality or would it need to go into the core? |
We thought about putting this functionality into a plugin as well - but that would mean that the env variables would not be available inside of yarnrc, because that is where yarn learns about the plugins in the first place. So it probably has to go into core. |
The config is loaded every time you run a Yarn command. So a change in I agree with you, @alubbe, it should go into core. |
Questions about this part. When you refer to "the functions that expect an env", would an example of one of those functions be something like I may be overthinking this but I wrote this and it looked a lot like the import {parse as dotenvParse} from 'dotenv';
...
private importDotenvSettings() {
const dotenvFile = this.values.get(`dotenvFilePath`);
if (dotenvFile) {
if (!xfs.existsSync(dotenvFile)) {
throw new Error(`The "dotenvPath" option has been set (in ${this.sources.get(`dotenvFilePath`)}), but the specified location doesn't exist (${dotenvFile}).`);
} else {
const parsedDotenvFile = dotenvParse(xfs.readFileSync(dotenvFile));
for (const [key, value] of Object.entries(parsedDotenvFile)) {
if (this.settings.has(key))
throw new Error(`Cannot redefine settings "${key}"`);
const definition: SettingsDefinition = {description: ``, default: null, type: SettingsType.STRING} ;
this.settings.set(key, definition);
this.values.set(key, value);
}
}
}
} |
Another concern about implementing this like how I describe above is that the values loaded from the env file would then be able to be mutated by doing I don't know what the right path forward is on this. |
The configuration env can't be modified using a plugin currently. We could perhaps add some kind of
Yep, there's already the external https://github.com/jeysal/yarn-plugin-dotenv/ that makes
It wouldn't,
The method that imports setting values is
I'll move the rest of this on Discord. |
I just wanted to chime in here and mention that I'm most interested in supporting this at a plugin level where there's more freedom to be opinionated about how |
I agree with @kherock after thinking about this more. A hook that provides configuration customization would probably be more flexible and less specific to just this use case. |
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
If you see three posts marked as spam because they just ask for update in a thread where updates would be publicly visible if there was any, posting a fourth one won't be very useful. I'm going to close this PR since the implementation isn't quite in line with what would be optimal. If someone's willing to implement in a new PR what Paul described here, we'll take another look. |
What's the problem this PR addresses?
Fixes #2724.
It was impossible to tell Yarn to load
.env
variables before running commands.Typical use case is for NPM auth tokens that may differ from a project to an other, you want them to be stored locally in your project rather than in a
.bashrc
file for example.Why not setting environment variables in a
.bashrc
/.zshrc
file? This makes your project rely on external (invisible to you) files, and defeat the purpose of Yarn to make projects amazingly self-contained. Plus, it makes it impossible to have project-specific variables.How did you fix it?
dotenv
as a dependency ofyarnpkg-cli
.dotenvPath
configuration key toyarnpkg-core/sources/Configuration.ts
.yarnpkg-cli/sources/main.ts
:gatsby/static/configuration/yarnrc.json
).➡️ How to use this setting?
And now, running
yarn run
or accessingprocess.env.NPM_AUTH_TOKEN
in a Node module (that's ran by Yarn) does give us the expectednpm_123
output.If there's no file at
dotenvPath
, an exception is thrown (similar to what happen whenyarnPath
is unresolvable).☀️ Note: if no
dotenvPath
is provided, nothing happens at all. So it's an opt-in, non-breaking feature. It won't bother current users who might have a.env
file but don't expect it to be checked in.Checklist