Add option to disable runtime tips#935
Conversation
There was a problem hiding this comment.
The feature is implemented cleanly, but the boolean precedence logic in lib/main.js is now inconsistent across tips vs debug/quiet, relying on || chaining of mixed string/boolean sources that’s brittle and harder to reason about. Tests for “no tip” behavior are slightly under-specified (they only assert some log occurred), which could make them more fragile as logging evolves. Addressing these will improve long-term maintainability and reduce regression risk.
Additional notes (2)
- Maintainability |
lib/main.js:255-255
configDotenvcurrently honors.env-providedDOTENV_CONFIG_TIPS(by re-reading it afterpopulate), but it does not do the same forDOTENV_CONFIG_QUIET/DOTENV_CONFIG_DEBUGprecedence relative tooptionsin a consistent, explicit way. With the current pattern (parseBoolean(processEnv.DOTENV_CONFIG_QUIET || quiet)), an explicit env value of'false'won’t override a previouslytruequietbecause'||'will fall back to the prior boolean when the env string is falsy-ish only after parsing (i.e.,'false'is truthy and works, but this pattern is brittle and inconsistent across settings).
Given you intentionally avoided normalizing at the boundary, it’s still worth making the precedence logic consistent and explicit for all three toggles (debug, quiet, tips) to avoid subtle future regressions and to match the documented “env takes precedence” behavior.
- Maintainability |
tests/test-config.js:11-11
The helperhasLoggedTipsearches all args for a string containing'-- tip:', which is good, but the tests only assertlogStub.called(not that it logged the injecting env message). This can become flaky if other logs occur (e.g., future debug logs, error logs, or other tests leaking logs) and still satisfy the assertion.
Tightening the assertion to specifically check the injecting-env line makes the test more resilient and better aligned with the feature under test.
Summary of changes
What this PR adds
- Introduces a new
tipsoption (defaulting totrue) to control whether runtime logs include the random-- tip: ...suffix. - Adds support for configuring tips via:
config({ tips: false })DOTENV_CONFIG_TIPS=false- CLI-style
dotenv_config_tips=false
Implementation details
- CLI parsing: Extends
lib/cli-options.jsregex to matchdotenv_config_tips=.... - Env parsing: Adds
DOTENV_CONFIG_TIPSpassthrough inlib/env-options.js. - Runtime behavior (
lib/main.js):- Defaults
tipstotrue. - Applies precedence: explicit env var
DOTENV_CONFIG_TIPS>options.tips. - Re-checks
DOTENV_CONFIG_TIPSafter.envpopulation to allow.envto affect the setting. - Builds the log message without the tip when
tips === false.
- Defaults
Docs & typings
- Updates
README.mdandREADME-es.mdwith a new##### tipssection. - Adds
tips?: booleantolib/main.d.tswith precedence notes.
Tests
- Adds CLI matcher coverage for
dotenv_config_tips=false. - Adds config tests verifying that tips are omitted when
tips: falseorDOTENV_CONFIG_TIPS=false.
Adds a
tipstoggle so consumers can omit the random-- tip: ...segment from the runtimeinjecting env ...log (without having to fully silence logs viaquiet).Changes
tipsoption support inconfig()plusDOTENV_CONFIG_TIPS/dotenv_config_tips=...support.tips: falseandDOTENV_CONFIG_TIPS=false.Verification
reviewChanges skipped: normalize boolean env/cli option parsing at the boundary (cli/env options currently forward strings for
quiet/debug/override; keeping that pattern avoids a broader refactor).Refs #901.