-
-
Notifications
You must be signed in to change notification settings - Fork 137
feat: add support to load env file #664
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
Conversation
✅ Deploy Preview for tsdown ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
commit: |
1cc0e6f to
92d78dc
Compare
sxzz
left a comment
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.
Use process.loadEnvFile instead of dotenv.
The main reason for using dotenv over This makes it impossible to distinguish between variables specifically defined in the file and those already present in the system environment. For example, if a user runs By using dotenv, we get an isolated object of just the file's contents. This allows us to precisely apply prefix filtering and handle precedence logic without accidentally picking up or re-processing system-level variables that shouldn't go through the resolution logic. |
|
@toto6038 I found |
src/config/options.ts
Outdated
| } | ||
|
|
||
| if (envFile) { | ||
| // MARK: - handling envFile |
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.
Remove comments
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.
fixed
src/config/options.ts
Outdated
| @@ -1,7 +1,9 @@ | |||
| import fs from 'node:fs' | |||
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.
| import fs from 'node:fs' | |
| import { readFile } from 'node:fs/promises' |
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.
fixed
src/config/options.ts
Outdated
| }) | ||
| } | ||
|
|
||
| function loadEnvFile(filePath: string, envPrefixes: string[], logger: Logger) { |
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.
| function loadEnvFile(filePath: string, envPrefixes: string[], logger: Logger) { | |
| function loadEnv(filePath: string, envPrefixes: string[], logger: Logger) { |
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.
Also read values from existing process.env
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.
If we want to read all variables from process.env, we could stick to process.env.loadEnvFile as we don't need to distinguish the variables specified in env file anymore.
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.
No, { ...parsedFromEnvFile (with prefixed), ...currentProcessEnv (with prefixed), ...envsFromTsdownOptions }
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.
I'd like to confirm the intended behavior of the following:
- Is loading process.env with prefix filtering exclusive to when envFile is provided?
- Should the override precedence be
--env> env file >process.env?
Thanks!
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.
Intended behaviors:
- Always load
process.env - Environment variables from env file and
process.envshould be filtered with prefix. - precedence
envoption (aka CLI--env, optional, highest priority)process.env(always load)- env file (aka CLI
--env-file, optional)
d51022c to
beecaa1
Compare
|
I have reworked the merging and filtering logic of environment variables. If things look good, I can proceed to elaborate this in documentation. |
Description
This PR introduces support for loading environment variables via a file, specifically
--env-fileand--env-prefixcommand line flags and corresponding config options.dotenvis used to parse env file, and variables are filtered by prefix (default isTSDOWN_) to prevent accidental exposure of sensitive information.Regarding variable priority, explicit CLI arguments take precedence. Specifically, if a user defines a variable using both the
--env.*flag and within an--env-filesimultaneously, the value provided via the--env.*flag will override the one in the file.Linked Issues
Resolves #652
Additional context
Note on Node.js Conflict: Since Node.js v20.6.0, the runtime includes a native
--env-fileflag. If a user provides an invalid path or forgets to use the--separator between Node and tsdown arguments, Node.js may intercept the flag and throw an error before tsdown even executes.