process: doc-only deprecate non-string env value #18990

TimothyGu · 2018-02-25T21:51:36Z

This PR provides an alternative approach to #18158 by deprecating the problematic behavior of process.env when a non-string is assigned to a property. I would like this deprecation to become Runtime in Node.js v11.

Refs: #15089
Refs: #18158

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

process

TimothyGu · 2018-02-25T21:53:05Z

CI: https://ci.nodejs.org/job/node-test-commit/16491/

addaleax

Thanks! I think this is a good approach.

addaleax · 2018-02-25T21:53:56Z

src/node.cc

+          "DEP01XX").IsNothing())
+      return;
+    env_nonstring_deprecation_warned = true;
+  }


Thank you for this, I would expect it to become pretty helpful when trying to figure out actual ecosystem usage.

addaleax · 2018-02-25T21:55:44Z

doc/api/process.md

@@ -867,7 +871,8 @@ console.log(process.env.foo);
 ```

 Assigning a property on `process.env` will implicitly convert the value
-to a string.
+to a string. **This behavior is deprecated.** Future versions of Node.js will


I’d replace will → may – at least I, personally, don’t feel comfortable committing to this.

addaleax · 2018-02-25T21:57:53Z

src/node.cc

@@ -2628,6 +2633,17 @@ static void EnvGetter(Local<Name> property,
 static void EnvSetter(Local<Name> property,
                      Local<Value> value,
                      const PropertyCallbackInfo<Value>& info) {
+  if (config_pending_deprecation && !env_nonstring_deprecation_warned &&
+      !value->IsString()) {


Can we also allow numbers + booleans, here and in the doc? I don’t see anything wrong with the well-defined behaviour we get from them, plus at least in the case of numbers doing this is pretty common (I count 3 test files in our suite alone).

👍 makes sense to me.

mcollina

LGTM.

Very good work.

jasnell

Few nits. Generally lgtm tho

jasnell · 2018-02-26T13:36:54Z

doc/api/deprecations.md

@@ -914,6 +914,16 @@ Type: Runtime

 This was never a documented feature.

+<a id="DEP01XX"></a>
+### DEP01XX: process.env string coercion


Just DEP00XX

We are already at DEP0100 :)

(Unless you meant DEP0XXX…)

Nope, I mean DEP00XX. The same tooling that looks for REPLACEME in the docs looks for that pattern also and will flag it on release if it's not set properly

Ah, in that case we should fix the tooling also…

Agreed, see nodejs/node-core-utils#193

jasnell · 2018-02-26T13:37:57Z

doc/api/deprecations.md

+<a id="DEP01XX"></a>
+### DEP01XX: process.env string coercion
+
+Type: Documentation-only (supports [`--pending-deprecation`][])


I'd move the supports --pending-deprecation to the description below.

@jasnell Just fyi, this was pretty recently added to the deprecation type on purpose in #18433

jasnell · 2018-02-26T13:39:15Z

src/node.cc

+// Set by EnvSetter when a warning is emitted for non-string assignment to
+// process.env, to prevent the same warning from being emitted multiple times
+// per process.
+bool env_nonstring_deprecation_warned = false;


Rather than add more global state can this be a flag on Environment?

jasnell · 2018-02-26T21:08:38Z

Why is this marked tsc-agenda?

TimothyGu · 2018-02-26T21:10:24Z

@jasnell Because #18158 is also. If we were to take off the label we should do the same to that PR.

mcollina · 2018-02-27T12:14:51Z

Removing tsc-agenda from here as well. We can leave this to github, there seems an agreement that this is a less contentious way to fix the problem.

TimothyGu · 2018-03-04T21:48:14Z

@addaleax and @jasnell: PTAL again

TimothyGu · 2018-03-04T21:52:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13493/

TimothyGu · 2018-03-06T06:39:37Z

I plan on landing this this week, either tomorrow or on Wednesday. Untagging tsc-review as many TSC members have already looked at this.

(I'll fix the deprecations.md merge conflict during landing.)

thefourtheye · 2018-03-06T06:42:56Z

src/node.cc

+      !value->IsString() && !value->IsNumber() && !value->IsBoolean()) {
+    if (ProcessEmitDeprecationWarning(
+          env,
+          "Assigning any value other than a string, number, or boolean value "


Nit: I am not a native English speaker, but I believe this message can be presented better. Unfortunately, I am also not sure how to improve it.

I'll remove the second "value" on landing. Hope that helps.

Thanks. That felt redundant to me, when I read that for the first time.

Also, does it sound better if we change the second part to, "If the value is not one of them, use the string representation of it."?

TimothyGu · 2018-03-07T18:44:24Z

Landed in 5826fe4.

PR-URL: #18990 Refs: #15089 Refs: #18158 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

This is a follow-up of #18990. A new deprecation id was assigned in it, but it was not reflected in code and test. PR-URL: #19209 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

PR-URL: nodejs#18990 Refs: nodejs#15089 Refs: nodejs#18158 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

This is a follow-up of nodejs#18990. A new deprecation id was assigned in it, but it was not reflected in code and test. PR-URL: nodejs#19209 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

TimothyGu added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 25, 2018

TimothyGu requested review from mcollina, targos and a team February 25, 2018 21:51

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 25, 2018

TimothyGu mentioned this pull request Feb 25, 2018

src: delete process.env values set to undefined #18158

Closed

4 tasks

TimothyGu added semver-major PRs that contain breaking changes and should be released in the next major version. process Issues and PRs related to the process subsystem. labels Feb 25, 2018

addaleax reviewed Feb 25, 2018

View reviewed changes

mcollina approved these changes Feb 26, 2018

View reviewed changes

lpinca approved these changes Feb 26, 2018

View reviewed changes

cjihrig approved these changes Feb 26, 2018

View reviewed changes

jasnell approved these changes Feb 26, 2018

View reviewed changes

mhdawson mentioned this pull request Feb 26, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-02-28 nodejs/TSC#496

Closed

mcollina added tsc-review and removed tsc-agenda Issues and PRs to discuss during the meetings of the TSC. labels Feb 27, 2018

gibfahn mentioned this pull request Feb 27, 2018

Check DEP00XX and REPLACEME tags for PRs nodejs/node-core-utils#193

Closed

TimothyGu force-pushed the doc-deprecate-process-env-non-string branch 2 times, most recently from 6fbae02 to 1b49210 Compare March 4, 2018 21:47

TimothyGu force-pushed the doc-deprecate-process-env-non-string branch from 1b49210 to dce66fe Compare March 4, 2018 21:48

process: doc-only deprecate non-string env value

e7af63b

TimothyGu force-pushed the doc-deprecate-process-env-non-string branch from dce66fe to e7af63b Compare March 4, 2018 21:52

addaleax approved these changes Mar 4, 2018

View reviewed changes

gibfahn approved these changes Mar 5, 2018

View reviewed changes

targos approved these changes Mar 5, 2018

View reviewed changes

Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 5, 2018

TimothyGu added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed tsc-review labels Mar 6, 2018

thefourtheye approved these changes Mar 6, 2018

View reviewed changes

TimothyGu closed this Mar 7, 2018

TimothyGu deleted the doc-deprecate-process-env-non-string branch March 7, 2018 18:44

thefourtheye mentioned this pull request Mar 7, 2018

src: fix deprecation id for non-string env value #19209

Closed

2 tasks

tniessen added the deprecations Issues and PRs related to deprecations. label Apr 28, 2018

bnoordhuis mentioned this pull request Jul 21, 2023

Setting undefined to process.env.* sets a "undefined" string value #48875

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

process: doc-only deprecate non-string env value #18990

process: doc-only deprecate non-string env value #18990

TimothyGu commented Feb 25, 2018

TimothyGu commented Feb 25, 2018

addaleax left a comment

addaleax Feb 25, 2018

addaleax Feb 25, 2018

addaleax Feb 25, 2018

TimothyGu Feb 25, 2018

targos Feb 27, 2018

mcollina left a comment

jasnell left a comment

jasnell Feb 26, 2018

TimothyGu Feb 27, 2018

jasnell Feb 27, 2018

TimothyGu Feb 27, 2018

gibfahn Feb 27, 2018

jasnell Feb 26, 2018

addaleax Feb 26, 2018

jasnell Feb 26, 2018

TimothyGu Feb 27, 2018

jasnell commented Feb 26, 2018

TimothyGu commented Feb 26, 2018

mcollina commented Feb 27, 2018

TimothyGu commented Mar 4, 2018

TimothyGu commented Mar 4, 2018

TimothyGu commented Mar 6, 2018 •

edited

Loading

thefourtheye Mar 6, 2018

TimothyGu Mar 7, 2018

thefourtheye Mar 7, 2018

TimothyGu commented Mar 7, 2018

process: doc-only deprecate non-string env value #18990

process: doc-only deprecate non-string env value #18990

Conversation

TimothyGu commented Feb 25, 2018

Checklist

Affected core subsystem(s)

TimothyGu commented Feb 25, 2018

addaleax 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

mcollina left a comment

Choose a reason for hiding this comment

jasnell 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

jasnell commented Feb 26, 2018

TimothyGu commented Feb 26, 2018

mcollina commented Feb 27, 2018

TimothyGu commented Mar 4, 2018

TimothyGu commented Mar 4, 2018

TimothyGu commented Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Mar 7, 2018

TimothyGu commented Mar 6, 2018 •

edited

Loading