-
Notifications
You must be signed in to change notification settings - Fork 637
[heft] Introduce a plugin to set environment variables. #3866
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
[heft] Introduce a plugin to set environment variables. #3866
Conversation
d9d36dd
to
41ef929
Compare
Nice 👍 I think once this merges, we can consider it the official solution to #2981 |
Is it better than just having Rush set the environment variable globally for the entire monorepo? |
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.
Needs updating to latest Heft
...-browserslist-warning-plugin/heft-suppress-browserslist-warning-plugin_2022-12-26-02-04.json
Outdated
Show resolved
Hide resolved
aa18a0d
to
9f56cee
Compare
2b66314
to
c64a82d
Compare
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.
Suggest to close this PR and instead implement the two easy features noted in the issue:
@iclanton and I discussed this in depth. We agreed on the following plan:
The motivation for implementing both solutions is that Microsoft is worried that even if |
common/changes/@rushstack/heft/heft-suppress-browserslist-warning-plugin_2024-02-20-01-13.json
Outdated
Show resolved
Hide resolved
Co-authored-by: David Michon <dmichon@microsoft.com>
5a2f1bb
to
cf97433
Compare
Co-authored-by: Pete Gonzalez <4673363+octogonz@users.noreply.github.com>
apps/heft/src/schemas/set-environment-variables-plugin.schema.json
Outdated
Show resolved
Hide resolved
apps/heft/src/schemas/set-environment-variables-plugin.schema.json
Outdated
Show resolved
Hide resolved
() => { | ||
for (const [key, value] of Object.entries(environmentVariablesToSet)) { | ||
taskSession.logger.terminal.writeLine(`Setting environment variable ${key}=${value}`); | ||
process.env[key] = value; | ||
} | ||
} |
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.
Do you think setting the environment variable explicitly should take a priority? Similarly to how dotenv
works? Seems like a dev could go into quite a weird debugging session otherwise.
process.env[key] ??= value;
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.
That's a good idea. Maybe we could split this setting into something like forcedEnvironmentVariables
versus defaultEnvironmentVariables
. I'm working on a design for common/config/rush/environment-variables.json for Rush that will include some other features, then we can adjust Heft's schema to align with that.
Summary
This PR introduces a new Heft plugin to suppress the warning printed by
browserslist
when it decides it's out-of-date.How it was tested
Tested by the build.