Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Jan 15, 2018

Before this commit, setting a property of process.env to undefined
would set the value to the string "undefined". This changes the
behavior to instead remove the value, working like the delete operator.

Refs: #15089

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 15, 2018
@targos targos force-pushed the process-env-undefined branch from 40e3915 to 6c63403 Compare January 15, 2018 14:05
@targos targos 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 Jan 15, 2018
@targos targos requested a review from a team January 15, 2018 14:06
src/node.cc Outdated
@@ -2715,6 +2715,18 @@ static void EnvGetter(Local<Name> property,
static void EnvSetter(Local<Name> property,
Local<Value> value,
const PropertyCallbackInfo<Value>& info) {
if (value->IsUndefined()) {
#ifdef __POSIX__
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to use EnvDeleter() instead of duplicating the code?

@targos
Copy link
Member Author

targos commented Jan 15, 2018

@cjihrig Not sure how. Two things to consider:

  • EnvDeleter ignores non-string properties whereas EnvGetter and EnvSetter convert them to strings.
  • EnvDeleter takes a const PropertyCallbackInfo<Boolean>& info parameter and I don't think I can construct one.

I pushed a change to move the code to another function, PTAL.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM.

@joyeecheung
Copy link
Member

mhdawson
mhdawson previously approved these changes Jan 15, 2018
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT - Removed my approval as I don't feel strongly enough that this should land given the objections.

@@ -899,6 +904,10 @@ process.env.TEST = 1;
delete process.env.TEST;
console.log(process.env.TEST);
// => undefined
process.env.TEST = 1;
process.env.TEST = undefined;
console.log(process.env.TEST);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, would it be better to show that the property TEST doesn't exist anymore in process.env, like in the test? As it is, it may not convey the idea clearly (setting undefined and then undefined is printed).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like this?

console.log('TEST' in process.env);
// => false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That's better, right?

Before this commit, setting a property of process.env to undefined
would set the value to the string "undefined". This changes the
behavior to instead remove the value, working like the delete operator.

Refs: nodejs#15089
@targos targos force-pushed the process-env-undefined branch from f49e58d to f88476a Compare January 23, 2018 12:14
@targos
Copy link
Member Author

targos commented Jan 23, 2018

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if this is semver-major, I still think this behavioral change is too significant to be done on process.env, especially when we previously documented to do the contrary. Plus, as #16961 demonstrated, we can never have a truly Correct environmental variable implementation with a string-based API. My opinion for this is, instead of adding exceptions to a previously internally consistent class, we should just leave process.env alone.

The current behavior is not without precedent either. The Web Storage API (better known as Local Storage and Session Storage) also cast undefined to string.

@mcollina
Copy link
Member

I'm adding this to the next TSC agenda.

@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jan 29, 2018
@Fishrock123
Copy link
Contributor

Setting undefined to undefined seem fine to me and is very consistent with how javascript generally works.

What about other non-string values? Shouldn't this throw or something?

> process.env.PWD = {}
{}
> process.env.PWD
'[object Object]'

What about null?

@jasnell
Copy link
Member

jasnell commented Jan 31, 2018

I'm torn on this one because I really like the idea of deleting but setting to undefined is probably the right thing. I'm reluctantly -1 on this but hopeful that a really convincing argument can convince me to switch that :-)

@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

I agree with @TimothyGu and I am also -1 on this.

@mhdawson
Copy link
Member

mhdawson commented Feb 5, 2018

This was discussed at the last TSC meeting. We are going to invite Timothy to this week's meeting and then call for a vote.

@rvagg
Copy link
Member

rvagg commented Feb 7, 2018

I like this, but without being able to attend to hear discussion I'll register an abstention for now unless it comes back for a vote.

@mhdawson
Copy link
Member

mhdawson commented Feb 7, 2018

Discussed in TSC meeting today. Still differing views, lets see if we can get Timothy at the next meeting before having a vote.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 19, 2018

Can someone on the endorsing-this-PR side speak to that?

I'm pretty much in the same boat as @addaleax. I think it should have worked like this from the start. If this PR doesn't land, I wouldn't be too upset though.

@joyeecheung
Copy link
Member

I don't feel strongly for this to land, although the comment in #18158 (comment) should be addressed

What about null?

It feels even stranger if we treat null and undefined differently.

@mhdawson mhdawson dismissed their stale review February 19, 2018 20:39

Don't feel strongly that this should land at this point given the objections

@gireeshpunathil
Copy link
Member

+1, better late than never. It is fair for a end user to expect

> process.env.LANG ? true : false
true
> process.env.LANG = undefined
undefined
> process.env.LANG ? true : false
false
>

rather than

> process.env.LANG ? true : false
true
> process.env.LANG = undefined
undefined
> process.env.LANG ? true : false
true
> 

because that is how normal objects behave:

> var foo = {}
undefined
> foo.bar = 'hello'
'hello'
> foo.bar ? true: false
true
> foo.bar=undefined
undefined
> foo.bar ? true: false
false
> 

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TimothyGu
Copy link
Member

@gireeshpunathil Your argument is one with a slippery slope. What makes undefined special compared to null in your argument? Problem is, this restriction on undefined is entirely arbitrary. That’s no good way to design an API.

We should instead focus on creating a better API in general that allows us to tackle #16961, rather than retrofitting suboptimal designs onto an object that has wide-spread compatibility impact.

@gireeshpunathil
Copy link
Member

What makes undefined special compared to null in your argument?

nothing, I don't discriminate null vs. undefined in this case - isn't it straightforward to fix null case as well, given we have the property setter?

Problem is, this restriction on undefined is entirely arbitrary. That’s no good way to design an API.

Existence of other, larger problem to be solved need not be a consideration for solving a problem for which fix is easy, right?

We should instead focus on creating a better API in general that allows us to tackle #16961, rather than retrofitting suboptimal designs onto an object that has wide-spread compatibility impact.

generally agree, but again, issue reported in #16961 need not hinder this from progressing, and both of them are not exclusive IMO.

@gireeshpunathil
Copy link
Member

@TimothyGu - on a second thought, I don't believe this will have a wide-spread compatibility impact. Can you explain potential implications of this change in the field?

@addaleax
Copy link
Member

What makes undefined special compared to null in your argument? Problem is, this restriction on undefined is entirely arbitrary.

@TimothyGu I’m in the “not too sad if this doesn’t happen” bucket, but I’m going to point out that many people (esp. language beginners) think of missing properties and properties with an undefined value as essentially the same – as in, setting a property to undefined is how you delete a property to them.

@mcollina
Copy link
Member

@TimothyGu

We should instead focus on creating a better API in general that allows us to tackle #16961, rather than retrofitting suboptimal designs onto an object that has wide-spread compatibility impact.

Are you proposing a new API to improve the situation? I'm not sure that will help. A lot of modules rely on process.env, and I think adding a new API to access the same data is not feasible. We can make surgical changes to edge cases.

I think we are not providing a good experience to anyone relying on process.env behavior for undefined or null.

Side note, I think we might even throw if you are setting an object.

@addaleax
Copy link
Member

Are you proposing a new API to improve the situation? I'm not sure that will help. A lot of modules rely on process.env, and I think adding a new API to access the same data is not feasible. We can make surgical changes to edge cases.

@mcollina A separate API is what is being suggested in #16961, although for a completely different reason.

@Fishrock123
Copy link
Contributor

This pull request has four Collaborators opposed to it

This is not true. I am not opposed to moving away from "undefined", but I would prefer having it set to undefined rather than deleting.

@targos
Copy link
Member Author

targos commented Feb 21, 2018

This is not true. I am not opposed to moving away from "undefined", but I would prefer having it set to undefined rather than deleting.

I can look into this. Don't know if this is actually possible because AFAIK process.env is not really an object but a sort of C++ proxy acting like one.

@Fishrock123
Copy link
Contributor

Hmmm, well it's mostly just a preference, "undefined" seems clearly wrong to me.

@addaleax
Copy link
Member

@Fishrock123 What would be stored in the actual environment in that case?

@TimothyGu
Copy link
Member

TimothyGu commented Feb 22, 2018

@mcollina

I think we might even throw if you are setting an object.

Now that is something I can get behind, and I believe that is better than both this PR and the status quo. And in fact I think we can and should start with documentation-only deprecation of setting non-string to process.env's properties.

The compatibility impact is difficult to measure however. @ChALkeR ;)


@addaleax

I’m going to point out that many people (esp. language beginners) think of missing properties and properties with an undefined value as essentially the same – as in, setting a property to undefined is how you delete a property to them.

I see what you are saying. However, I'd like to contend that this PR doesn't help with the general issue of making process.env more friendly to language beginners. As a mentor to many Node.js and JavaScript workshops at my university, I have first-hand experience in the fact that an API where there are few or none exceptions is a much easier to teach and learn than one where certain values are considered "special." It's much easier to explain that "process.env converts everything to strings" than "process.env converts null, numbers, objects, and even functions to strings, but not undefined, because we consider undefined to be special."

Even worse, I understand that a lot of Node.js developers come from other JavaScript-based platforms, like the browser world, where localStorage, sessionStorage, and element.dataset have exactly the same behavior as process.env currently does with regards to stringification. Now, instead of explaining to them that "process.env works just like localStorage", we are deviating from behavior from both the browser world and the past to create one with arbitrary exceptions. And that's not okay.

For that reason, I believe throwing an error when setting non-string to process.env would be both more internally consistent and friendlier for people starting out.


@Fishrock123

I am not opposed to moving away from "undefined", but I would prefer having it set to undefined rather than deleting.

That would be even odder given the current behavior and implementation of process.env.


@gireeshpunathil

I don't believe this will have a wide-spread compatibility impact. Can you explain potential implications of this change in the field?

The point is that the impact will for sure be non-zero. And this, coupled with other disadvantages I pointed out before, forms the body of my objection.

@mcollina
Copy link
Member

Now that is something I can get behind, and I believe that is better than both this PR and the status quo. And in fact I think we can and should start with documentation-only deprecation of setting non-string to process.env's properties.

I'm definitely 👍 on this approach. If you are setting process.env values that are not strings, it will should. I think we could even runtime-deprecate this in 10, but I'm ok to do it in 11 as well.

@TimothyGu TimothyGu changed the title src: delete proccess.env values set to undefined src: delete process.env values set to undefined Feb 25, 2018
@TimothyGu
Copy link
Member

I have proposed my approach to this issue in #18990.

@mcollina mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 27, 2018
@mcollina
Copy link
Member

I think this can be closed in favor of #18990.
I've removed the tsc-agenda label.

@targos
Copy link
Member Author

targos commented Feb 27, 2018

No objection to closing from me.

@targos targos closed this Feb 27, 2018
TimothyGu added a commit that referenced this pull request Mar 7, 2018
PR-URL: #18990
Refs: #15089
Refs: #18158
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18990
Refs: nodejs#15089
Refs: nodejs#18158
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@targos targos deleted the process-env-undefined branch October 17, 2020 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.