src: make parsing compatible with motdotla/dotenv package by marekpiechut · Pull Request #54215 · nodejs/node · GitHub

Review requested:

@nodejs/gyp

Hmm, no idea which subsystem to use here 🤔. I see that previous commits in these files were marked as src. Should I also use that?

A more readable way:

input.remove_prefix(1);
input.remove_suffix(2);

Fixed

Can you add a comment to what this function does?

Done

expanded is a string now. std::string::npos is the correct return value here.

👍Done, thanks

Can you add a documentation to here?
Returning the else statement early will make this function much more readable.

Done

You removed all of the comments in this function, and it is a lot less readable right now. I can't review it with the current state.

This is basically a new function. Old one was doing a lot of back & forth searching for chars and newlines in string and it was hard for me to fit in a fix. This goes through the content of file only once and parses it char by char.

This can return std::string_view since the underlying store will not be disposed.

Suggested change

      
            std::optional<std::string> Dotenv::GetValue(const std::string_view key) const {
          
            std::optional<std::string_view> Dotenv::GetValue(const std::string_view key) const {

Done

Suggested change

      
                return std::optional<std::string>{match->second};
          
                return match->second;

Done

Suggested change

      
            // These are no longer true, parser is now more strict when it comes to balancing double quotes
          
            // assert.strictEqual(process.env.MULTI_NOT_VALID_QUOTE, '"');
          
            // assert.strictEqual(process.env.MULTI_NOT_VALID, 'THIS');

Done

Codecov Report

❌ Patch coverage is 96.20253% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.09%. Comparing base (67f7137) to head (00fa796).
⚠️ Report is 5083 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_dotenv.cc	96.20%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #54215   +/-   ##
=======================================
  Coverage   87.09%   87.09%           
=======================================
  Files         647      647           
  Lines      181845   181900   +55     
  Branches    34913    34924   +11     
=======================================
+ Hits       158373   158426   +53     
+ Misses      16751    16749    -2     
- Partials     6721     6725    +4

Files with missing lines	Coverage Δ
src/node_dotenv.h	100.00% <ø> (ø)
src/node_dotenv.cc	84.35% <96.20%> (+3.98%)	⬆️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Hey @anonrig did you have a chance to take a second look? I've resolved all small things and only thing left is the parsing function that I have rewritten.

If you think that the change is too risky I can revert it, keep the tests and try to fit in the fix in current implementation.

This appears to be waiting for feedback from @nodejs/config.

dotenv: make parsing compatible with motdotla/dotenv package

5e4b537

Fixes: nodejs#54134

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 5, 2024

marekpiechut mentioned this pull request Aug 5, 2024

--env-file does not support inner quotes (does not behave like dotenv) #54134

Open

anonrig reviewed Aug 5, 2024

View reviewed changes

marekpiechut changed the title dotenv: make parsing compatible with motdotla/dotenv package src: make parsing compatible with motdotla/dotenv package Aug 5, 2024

marekpiechut added 2 commits August 5, 2024 18:06

cleanup & add comments

1456714

Fix linter issues

00fa796

avivkeller added the dotenv Issues and PRs related to .env file parsing label Aug 5, 2024

tniessen requested a review from anonrig June 26, 2025 15:05

dario-piotrowicz mentioned this pull request Jul 13, 2025

doc: add new environment variables doc page #59052

Merged

+                auto first = input.front();
+                if ((first == '\'' || first == '"' || first == '`') &&
+                    input.back() == first) {
+                  input = input.substr(1, input.size() - 2);

+                  // Expand \n to newline in double-quote strings
+                  size_t pos = 0;
+                  auto expanded = std::string(trimmed);
+                  while ((pos = expanded.find("\\n", pos)) != std::string_view::npos) {

-              void Dotenv::ParseContent(const std::string_view input) {
-                std::string lines(input);
+              std::string_view parse_key(std::string_view key) {

+                if (value.empty()) return "";
+                auto trimmed = trim_quotes(value);
+                if (value.front() == '\"' && value.back() == '\"') {

+                std::string::size_type start = 0;
+                std::string::size_type end = 0;
+                for (std::string::size_type i = 0; i < input.size(); i++) {

	std::optional<std::string> Dotenv::GetValue(const std::string_view key) const {
	std::optional<std::string_view> Dotenv::GetValue(const std::string_view key) const {

+                auto match = store_.find(key.data());
+                if (match != store_.end()) {
+                  return std::optional<std::string>{match->second};

	return std::optional<std::string>{match->second};
	return match->second;

src: make parsing compatible with motdotla/dotenv package by marekpiechut · Pull Request #54215 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: make parsing compatible with motdotla/dotenv package - #54215

src: make parsing compatible with motdotla/dotenv package#54215marekpiechut wants to merge 3 commits intonodejs:mainnodejs/node:mainfrom marekpiechut:env-inner-quotesmarekpiechut/node:env-inner-quotesCopy head branch name to clipboard

Conversation

marekpiechut commented Aug 5, 2024

Uh oh!

nodejs-github-bot commented Aug 5, 2024

Uh oh!

marekpiechut commented Aug 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marekpiechut commented Aug 28, 2024

Uh oh!

tniessen commented Apr 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

src: make parsing compatible with motdotla/dotenv package#54215
marekpiechut wants to merge 3 commits into
nodejs:mainfrom
marekpiechut:env-inner-quotes

codecov Bot commented Aug 5, 2024 •
edited
Loading