Skip to content

[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

Merged

Conversation

iclanton
Copy link
Member

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.

@iclanton iclanton force-pushed the heft-suppress-browserslist-warning-plugin branch from d9d36dd to 41ef929 Compare December 26, 2022 02:08
@elliot-nelson
Copy link
Collaborator

Nice 👍

I think once this merges, we can consider it the official solution to #2981

@octogonz
Copy link
Collaborator

Is it better than just having Rush set the environment variable globally for the entire monorepo?

Copy link
Contributor

@dmichon-msft dmichon-msft left a 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

@iclanton iclanton force-pushed the heft-suppress-browserslist-warning-plugin branch 2 times, most recently from aa18a0d to 9f56cee Compare February 19, 2024 17:16
@iclanton iclanton enabled auto-merge February 19, 2024 18:33
@iclanton iclanton force-pushed the heft-suppress-browserslist-warning-plugin branch from 2b66314 to c64a82d Compare February 19, 2024 18:50
Copy link
Collaborator

@octogonz octogonz left a 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:

#2981 (comment)

@octogonz
Copy link
Collaborator

octogonz commented Feb 19, 2024

Suggest to close this PR and instead implement the two easy features noted in the issue:

#2981 (comment)

@iclanton and I discussed this in depth. We agreed on the following plan:

  1. Add a Rush feature to force environment variables, as proposed in [all] Rushstack CI blocked by "browerslist update" warning #2981
  2. Rename heft-suppress-browserslist-warning-plugin to something like heft-env-variable-plugin, that will instead work by forcing environment variables such as BROWSERSLIST_IGNORE_OLD_DATA=1 without specifically focusing on that one package. It can optionally read and apply Rush's configuration.
  3. Add a website doc explaining why BROWSERSLIST_IGNORE_OLD_DATA=1 is a best practice for large monorepos

The motivation for implementing both solutions is that Microsoft is worried that even if rush and rushx don't print the warning, if an engineer sees the warning when invoking heft build they may still waste time trying to investigate a non-problem.

@iclanton iclanton changed the title Introduce heft-suppress-browserslist-warning-plugin. [heft] Introduce a plugin to set environment variables. Feb 20, 2024
@iclanton iclanton force-pushed the heft-suppress-browserslist-warning-plugin branch from 5a2f1bb to cf97433 Compare February 20, 2024 01:44
Co-authored-by: Pete Gonzalez <4673363+octogonz@users.noreply.github.com>
@iclanton iclanton merged commit 75bd106 into microsoft:main Feb 20, 2024
Comment on lines +27 to +32
() => {
for (const [key, value] of Object.entries(environmentVariablesToSet)) {
taskSession.logger.terminal.writeLine(`Setting environment variable ${key}=${value}`);
process.env[key] = value;
}
}
Copy link
Contributor

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;

Copy link
Collaborator

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.

@iclanton iclanton deleted the heft-suppress-browserslist-warning-plugin branch August 21, 2024 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants