doc: add environment variables specification #52735

IlyasShabi · 2024-04-28T15:21:41Z

Following #49148
Add documentation for environment variables specifications

anonrig · 2024-04-28T15:46:47Z

doc/api/process.md

+
+6. **Multiline values**:
+
+   * Values enclosed in double, single, or backtick quotes that span multiple


Suggested change

* Values enclosed in double, single, or backtick quotes that span multiple

* Values enclosed in quotes described above that span multiple

It may also make sense to mention that unlike shells " and ' behave identially. In shells typically ' doesn't interpolate anything, " allows inline interpolation and backticks execute a command in a subshell.

anonrig · 2024-04-28T15:47:31Z

doc/api/process.md

+   ```cjs
+   const assert = require('node:assert');
+   const process = require('node:process');
+   assert.strictEqual(process.env.MULTI_DOUBLE_QUOTED, 'double\nquoted');


You need to escape all \ characters

Suggested change

assert.strictEqual(process.env.MULTI_DOUBLE_QUOTED, 'double\nquoted');

assert.strictEqual(process.env.MULTI_DOUBLE_QUOTED, 'double\\nquoted');

In fact, I got the values from test files https://github.com/nodejs/node/blob/main/test/parallel/test-dotenv.js#L72

doc/api/process.md

GeoffreyBooth

I have two general concerns with this PR:

This is a lot of text to introduce and then need to maintain; we’re essentially defining our own specification here. Is this specified somewhere already that we can just link to?
What happens when we want to add a new feature that’s not specified, like string interpolation? We just update this spec? Would that be a breaking change?

GeoffreyBooth · 2024-04-28T17:27:34Z

doc/api/process.md

+
+   ```bash
+       # COMMENTS=work
+       INLINE_COMMENTS=inline comments # work


Is the space after comments part of the value of INLINE_COMMENTS?

@GeoffreyBooth No, we trim spaces in both the key and the value

GeoffreyBooth · 2024-04-28T17:28:31Z

doc/api/process.md

+This section describes how the environment variables file parser reads a file
+and sets up the environment variables in Node.js.
+
+1. **Basic parsing**:


The docs generally avoid bold. Rather than this being a numbered list you could make subheadings:

Suggested change

1. **Basic parsing**:

#### Basic parsing

GeoffreyBooth · 2024-04-28T17:29:19Z

doc/api/process.md

+   assert.strictEqual(process.env.INLINE_COMMENTS, 'inline comments');
+   ```
+
+3. **Whitespace handling**:


I would list this second, above comments.

GeoffreyBooth · 2024-04-28T17:31:35Z

doc/api/process.md

+   * Any `export` keyword before a key is ignored, allowing compatibility with
+     shell scripts.


I initially read this as saying that any lines beginning with export get ignored, not that just the export keyword alone is ignored.

Suggested change

* Any `export` keyword before a key is ignored, allowing compatibility with

shell scripts.

* Any `export` keyword before a key is ignored, allowing compatibility with

shell scripts. The line is parsed as if `export` wasn't there.

anonrig · 2024-05-23T23:18:04Z

@IlyasShabi are you interested in continuing on this? this is the only missing PR from making dotenv stable.

IlyasShabi · 2024-05-24T09:28:26Z

@anonrig sorry for the delay I was on holidays, yes will work on it this weekend.

IlyasShabi · 2024-05-25T22:07:18Z

@GeoffreyBooth AFAIK, the only documentation for parsing env files is from the dotenv package https://github.com/motdotla/dotenv

For adding new features we should update this document. If the changes are breaking, we need to document them clearly.
Do you have some suggestions ?

avivkeller

removed

anonrig · 2024-05-26T04:05:23Z

@redyetidev your comment is not really helpful or contain any actionable item. please provide direct suggestions.

anonrig · 2024-05-26T04:06:26Z

I have two general concerns with this PR:

This is a lot of text to introduce and then need to maintain; we’re essentially defining our own specification here. Is this specified somewhere already that we can just link to?

What happens when we want to add a new feature that’s not specified, like string interpolation? We just update this spec? Would that be a breaking change?

@GeoffreyBooth there is no direct specification for dotenv other than the documentation. i believe this is the first technical specification of it. i'd like to move with this specification. do you have any blockers?

avivkeller · 2024-05-26T11:13:38Z

@redyetidev your comment is not really helpful or contain any actionable item. please provide direct suggestions.

Sorry! I'll do a review instead. I didn't mean for it to come off as unhelpful.

avivkeller

Tip

I'm not a core collaborator, so these are only my suggestions (and are non-blocking)

avivkeller · 2024-05-26T12:56:29Z

doc/api/process.md

@@ -2309,6 +2309,227 @@ import { loadEnvFile } from 'node:process';
 loadEnvFile();
 ```

+### Environment variables file parser specification


Maybe rename this to Environment Variable File Parsing Specification, or something similar?

IMO the same should use a -ing word

avivkeller · 2024-05-26T12:57:14Z

doc/api/process.md

@@ -2309,6 +2309,227 @@ import { loadEnvFile } from 'node:process';
 loadEnvFile();
 ```

+### Environment variables file parser specification
+
+This section describes how the environment variables file parser reads a file


Similarly, maybe replace references to a 'environment variables file parser' to just the 'parser', as it is already noted in the title.

doc/api/process.md

avivkeller · 2024-05-26T13:00:28Z

doc/api/process.md

+#### Comments
+
+* Lines starting with `#` are treated as comments and ignored.
+* Inline comments (starting with `#` after a value) are ignored, and do not


these are not inline comments, they are just comments

"A normal" // comment "An inline" /* comment */ "can have more values afterward"

doc/api/process.md

IlyasShabi · 2024-06-02T10:40:24Z

@anonrig IMO this PR is ready to land

avivkeller

A few nitpicks, but otherwise LGTM

(I'm not a core collaborator, so this is non-blocking)

doc/api/process.md

anonrig · 2024-06-09T14:51:41Z

hey @IlyasShabi can you apply/address the reviews?

doc/api/process.md

avivkeller

I have a few notes, but as a triage member, they are non-blocking

avivkeller · 2024-06-19T22:04:46Z

doc/api/process.md

+This section describes how environment variables file parser reads a file
+and sets up the environment variables in Node.js.


I think this can be reworded to:
This section describes how environment variables are read and parsed from a file.

avivkeller · 2024-06-19T22:05:43Z

doc/api/process.md

+This section describes how environment variables file parser reads a file
+and sets up the environment variables in Node.js.
+While `.env` files descend from shell scripts that export environment
+variables, there are important distinctions in how they handle quoting,


IMO, instead of "they" vs "us", you can write it in way that speaks in general

avivkeller · 2024-06-19T22:06:34Z

doc/api/process.md

+and sets up the environment variables in Node.js.
+While `.env` files descend from shell scripts that export environment
+variables, there are important distinctions in how they handle quoting,
+spacing, and escaping values.


IMO, you should list these items like so:
... various ____, such as ...

doc/api/process.md

avivkeller · 2024-06-19T22:09:34Z

doc/api/process.md

+
+#### Basic parsing
+
+* The parser processes input until it finds a newline, splitting each part into


Remember to use active sentence style, so wording like "Input data is processed ..." is preferred

avivkeller · 2024-06-19T22:10:04Z

doc/api/process.md

+
+```bash
+    # COMMENTS=work
+    INLINE_COMMENTS=inline comments # work


avivkeller · 2024-06-19T22:10:22Z

doc/api/process.md

+
+#### Empty values
+
+* Variables with no value assigned (or example, a key followed by `=`) are set to


"for" examples

avivkeller · 2024-06-19T22:10:42Z

doc/api/process.md

+
+#### Empty values
+
+* Variables with no value assigned (or example, a key followed by `=`) are set to


"for" example ...

avivkeller · 2024-06-19T22:11:38Z

doc/api/process.md

+
+#### Quoted values
+
+* Single (`'`), double (`"`), and backtick (`` ` ``) quotes are recognized.


I think this shouldn't be a bullet

avivkeller · 2024-06-19T22:12:54Z

doc/api/process.md

+const assert = require('node:assert');
+const process = require('node:process');
+assert.strictEqual(process.env.EXPAND_NEWLINES, 'expand\nnew\nlines');
+assert.strictEqual(process.env.DONT_EXPAND_UNQUOTED,
+                   'dontexpand\\nnewlines');
+assert.strictEqual(process.env.DONT_EXPAND_SQUOTED,
+                   'dontexpand\\nnewlines');


IMO, assertions shouldn't be used in the example, but rather just a comment explaining the item's value

nodejs-github-bot added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Apr 28, 2024

anonrig requested a review from GeoffreyBooth April 28, 2024 15:44

anonrig reviewed Apr 28, 2024

View reviewed changes

benjamingr reviewed Apr 28, 2024

View reviewed changes

doc/api/process.md Show resolved Hide resolved

GeoffreyBooth reviewed Apr 28, 2024

View reviewed changes

IlyasShabi force-pushed the load-env-spec branch from 969dcef to 24024ce Compare May 25, 2024 22:04

avivkeller suggested changes May 26, 2024

View reviewed changes

IlyasShabi force-pushed the load-env-spec branch from 24024ce to 4e13f43 Compare June 2, 2024 10:38

avivkeller suggested changes Jun 2, 2024

View reviewed changes

doc/api/process.md Outdated Show resolved Hide resolved

doc/api/process.md Show resolved Hide resolved

doc/api/process.md Outdated Show resolved Hide resolved

anonrig reviewed Jun 9, 2024

View reviewed changes

doc/api/process.md Outdated Show resolved Hide resolved

doc/api/process.md Outdated Show resolved Hide resolved

doc/api/process.md Outdated Show resolved Hide resolved

doc/api/process.md Outdated Show resolved Hide resolved

doc/api/process.md Outdated Show resolved Hide resolved

doc: add environment variables specification

ce0154b

IlyasShabi force-pushed the load-env-spec branch from 4e13f43 to ce0154b Compare June 11, 2024 19:19

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

avivkeller suggested changes Jun 19, 2024

View reviewed changes


		6. Multiline values:

		* Values enclosed in double, single, or backtick quotes that span multiple

	* Values enclosed in double, single, or backtick quotes that span multiple
	* Values enclosed in quotes described above that span multiple

	assert.strictEqual(process.env.MULTI_DOUBLE_QUOTED, 'double\nquoted');
	assert.strictEqual(process.env.MULTI_DOUBLE_QUOTED, 'double\\nquoted');

		* Any `export` keyword before a key is ignored, allowing compatibility with
		shell scripts.

		This section describes how environment variables file parser reads a file
		and sets up the environment variables in Node.js.


		#### Basic parsing

		* The parser processes input until it finds a newline, splitting each part into


		#### Empty values

		* Variables with no value assigned (or example, a key followed by `=`) are set to


		#### Quoted values

		* Single (`'`), double (`"`), and backtick (`` ` ``) quotes are recognized.

doc: add environment variables specification #52735

Are you sure you want to change the base?

doc: add environment variables specification #52735

Conversation

IlyasShabi commented Apr 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anonrig commented May 23, 2024

IlyasShabi commented May 24, 2024

IlyasShabi commented May 25, 2024

avivkeller left a comment • edited Loading

Choose a reason for hiding this comment

anonrig commented May 26, 2024

anonrig commented May 26, 2024

avivkeller commented May 26, 2024

avivkeller left a comment

Choose a reason for hiding this comment

avivkeller May 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avivkeller May 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IlyasShabi commented Jun 2, 2024

avivkeller left a comment

Choose a reason for hiding this comment

anonrig commented Jun 9, 2024

avivkeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IlyasShabi commented Apr 28, 2024 •

edited

Loading

avivkeller left a comment •

edited

Loading

avivkeller May 26, 2024 •

edited

Loading

avivkeller May 26, 2024 •

edited

Loading