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

feat: add support for unsetEnv config option #19439

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KKSzymanowski
Copy link

@KKSzymanowski KKSzymanowski commented Feb 16, 2025

Description

This adds a new config option unsetEnv which controls how the vite build command behaves when an unset environment variable is encountered.

This aims to protect against missing variables when performing a production build. These issues could be caught by a schema validation approach but only at runtime after the new version has been deployed. If unsetEnv would be set to error this would fail the build and the whole pipeline preventing the deployment.

This is backwards compatible, as the default value is off and doesn't alter the build process if left unchanged.

I'm aware of the argument made by @bluwy in #8021 but I believe this isn't a concern here because:

  1. This is off by default
  2. If not off, it can be disabled in the config.
  3. Keeping it enabled and adding an intentionally undefined variable would be caught at build time without risk of any production issues.
  4. If keeping it enabled is preferable but the developer still wants to have a deliberately undefined env variable they can designate a special string value to the variable (such as 'undefined') and cast it to undefined in code.
    As a side note, I believe deliberately omitting an env variable isn't the best practice as it suggests a mistake in the configuration rather than an intentional action.

Dev build

While this PR adds unset env variable check in production build it does nothing during development. If you feel like extending this behaviour to the dev build is desired I can add it to the PR. I briefly tested the Proxy approach mentioned by @bluwy in #8021 and it can be done although it has two limitations:

  1. It cannot fail the build, at most throw an error or write a console.error in the browser.
  2. The file using the missing env variable has to be loaded for it to catch the issue. The missing env variable has to be accessed for it to catch the issue.

Further considerations

  • Set it to warn by default? Still maintains BC with regard to output files and success/failure of the build but changes the build console output.
  • Add a sentence explaining how to disable the error/warning in the error/warning message itself?

@sapphi-red sapphi-red added the p2-to-be-discussed Enhancement under consideration (priority) label Feb 25, 2025
@sapphi-red
Copy link
Member

sapphi-red commented Mar 19, 2025

Thanks for the PR.

In the last team meeting, we discussed about this PR and concluded that this new option does not match our criteria.
https://github.com/vitejs/vite/blob/main/CONTRIBUTING.md#think-before-adding-yet-another-option

If the user needs to enable it by an option, it isn't much different with checking it in the config on their own (e.g. if (!process.env.something) throw "should exist") (this would make the build fail) or adding a plugin that achieves this functionality. So if we are adding this to core, we should aim to eventually enable this by default. However, it is common to have a code switching features depending on the existence of import.meta.env.something. To filter out the false positives, we need to add an allowlist option and users will have to configure it. That isn't much different from manually checking in the config.

For users that are fine with loose checks, they can use the stricter types (#19077). For users that wants more stricter checks, they can manually check it in the config or make a plugin has a similar functionality with this PR.

@KKSzymanowski
Copy link
Author

Thanks for looking into this and I see your point.

I'll look into the plugin route although I'm not sure it can be done without duplicating a lot of code from the core relating to env variable handling.
Do you think it would it be feasible to fire a new hook every time an unknown env variable is encountered (in that callback in replaceAll()`) and let plugins hook into that and do whatever they want (eg. throw an error)?

@sapphi-red
Copy link
Member

I think you can find the code using import.meta with es-module-lexer and use that to find all import.meta.env.*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants