fix(dotenv): load .env files with Next.js precedence#70
Conversation
- Document Vite double-loading interaction in loadDotenv JSDoc
- Fix escaped dollar sign bug ($100 was not unescaped when $ wasn't
followed by a valid variable name)
- Add type cast for parseEnv return value
- Expand test coverage: loadedEnv assertions, production mode
precedence, ${VAR} expansion, escaped dollars, circular refs,
missing files, empty result
- Add getDotenvFiles unit tests for all three modes
- Add 'Prefer Node.js Built-in APIs' guideline to AGENTS.md
Co-authored-by: liuxiaopai-ai <liuxiaopai-ai@users.noreply.github.com>
commit: |
|
|
/bigbonk review |
There was a problem hiding this comment.
Review
Good implementation overall. The precedence order, variable expansion, Vite interaction model, and test coverage are solid. A few issues to flag:
Bug: Cross-file variable expansion doesn't work
When a variable in .env references a variable defined in .env.development.local, the expansion resolves to empty string instead of the actual value.
The expandEnv function constructs its context from parsed (current file) + processEnv. But processEnv only contains vars from previously loaded files that had different keys. Variables from the same file are correctly expanded via the recursive resolveValue, but cross-file forward references within a single expandEnv call won't pick up vars that were loaded into processEnv from earlier files if those vars were also defined in the current file (since ...parsed comes first, overriding them).
Wait, actually re-reading: { ...parsed, ...processEnv } means processEnv wins over parsed for the same key, which is correct (higher-priority files loaded first populate processEnv, lower-priority file's parsed values for the same key are ignored in context). And for references to keys that only exist in processEnv (from earlier files), they'll be found in context correctly.
So this is actually fine. The context merge order is correct. My mistake — disregard.
Bug: expanded[key] cache returns "" for empty strings too early
Line 100-101:
const cached = expanded[key];
if (cached !== undefined) return cached;If a variable legitimately resolves to "" (empty string), it gets cached as "" in expanded. On subsequent lookups for the same key, "" !== undefined is true, so it returns "". This is correct behavior. No issue here.
Real issue: context object conflates file vars with process env vars
const context: Record<string, string | undefined> = {
...parsed,
...processEnv,
};Since processEnv spreads second, process.env values override parsed file values in the expansion context. This is correct for the value used during expansion. But resolveValue is only called for keys in parsed (line 127), so process.env-only vars are never expanded themselves — which is also correct (they shouldn't be).
However, there's a subtle issue: context[key] on line 107 reads the raw value for process.env vars (which may themselves contain $VAR references). If a file var A=$B references a process.env var B=\$100, the resolveValue for B returns \$100 → after replace(ENV_REF_RE, ...) this stays \$100 → then value.replace(/\\\$/g, "$") turns it to $100. But wait, B is in processEnv, so its raw value in context is already \$100. Actually, parseEnv would have processed the escape in the file. In processEnv, if the user set B=\$100 in their shell, process.env.B would literally be \$100. The expansion would then incorrectly unescape it to $100.
This is a real (minor) issue: the \$ → $ unescape on line 119 runs on resolved values for all keys found in context, including process.env vars. If a process.env value literally contains \$, it will be silently unescaped. This matches dotenv-expand behavior though, so it's probably fine in practice. Just worth documenting.
Missing test: cross-file expansion
There's no test for a variable in .env that references a variable defined only in .env.development.local. For example:
# .env.development.local
DB_HOST=localhost
# .env
DB_URL=postgres://$DB_HOST/mydb
This should work based on the implementation (DB_HOST would be in processEnv by the time .env is processed), but it's not tested.
Missing test: multi-line values
node:util parseEnv handles quoted multi-line values. No tests cover this. Not a blocker but worth adding for confidence.
Minor: processEnv type narrowing
Line 67:
const parsed = parseEnv(fileContent) as Record<string, string>;The type cast is noted in the PR description. parseEnv returns object per Node.js typings, so the cast is necessary. This is fine.
The deploy command loads dotenv twice
In cli.ts line 350-388, deployCommand() doesn't call loadDotenv itself — it delegates to runDeploy which calls loadDotenv at deploy.ts:701. But if someone calls deploy() directly as a library function, it works correctly. And deployCommand() in the CLI doesn't double-load. This is fine.
Vite interaction explanation
The JSDoc comment on lines 37-52 is excellent and accurately describes the interaction. One nuance: Vite loads .env files and populates import.meta.env — but it also populates process.env in SSR mode (via loadEnv with prefixes: [''] when configured). If a user has envPrefix: '' in their vite config, Vite would expose all env vars to import.meta.env, which could conflict. This is an edge case that doesn't need to be handled now, but worth keeping in mind.
AGENTS.md change
The "Prefer Node.js Built-in APIs" guideline addition is clean and well-placed. The parseEnv example is directly relevant.
README change
The documentation in README.md is thorough and follows the existing style. The load order table, mode mapping, and override behavior are all clearly explained.
Summary
This is a solid implementation. The code is clean, well-documented, and the test suite covers the important cases. The use of node:util parseEnv over dotenv is the right call.
Suggestions for follow-up (non-blocking):
- Add a cross-file expansion test
- Add a multi-line value test
- Consider whether the
\$unescape should only apply toparsedkeys (not processEnv keys passed through expansion)
LGTM with the caveat that the two missing test cases above would strengthen confidence.
|
Review posted and approved on PR #70. Here's the summary: Verdict: Approved (LGTM) The implementation is solid. Key findings:
Suggested follow-ups (non-blocking):
|
Address review suggestions: test that .env can reference vars from higher-priority .env.development.local, and that parseEnv handles quoted multi-line values correctly.
Summary
Closes #24. Supersedes #46 (takes over @liuxiaopai-ai's work and addresses review feedback).
Adds automatic
.envfile loading with Next.js-compatible precedence for all CLI commands (dev,build,start,deploy).Load order (highest priority first)
process.envvalues (shell/CI).env.<mode>.local.env.local(skipped intestmode).env.<mode>.envSecurity model
NEXT_PUBLIC_*vars are inlined into client bundles via Vite'sdefine(the existinggetNextPublicEnvDefines()mechanism). Non-public vars stay server-only.next.config.jsenvproperty still bypasses the prefix filter (matches Next.js behavior).$VAR/${VAR}) supported with cycle detection.Changes from PR #46
process.envfor server-side access; Vite populatesimport.meta.env.VITE_*for its own system)\$100was not being unescaped because$followed by a digit doesn't match the variable-name regexparseEnvreturn valueloadedEnvassertions, production mode precedence,${VAR}expansion syntax, escaped dollars, circular references, missing files, empty result,getDotenvFilesfor all modesnode:utilparseEnvrather thandotenvpackage)Co-authored-by: liuxiaopai-ai liuxiaopai-ai@users.noreply.github.com