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: allow getting Symbols on process.env #9631

Closed
wants to merge 1 commit into
base: master
from

Conversation

@addaleax
Member

addaleax commented Nov 16, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

process

Description of change

1aa595e introduced a throw for accessing Symbol properties of process.env. However, this breaks util.inspect(process) and things like Object.prototype.toString.call(process.env), so this patch changes the behaviour for the getter to just always return undefined.

/cc @cjihrig

src: allow getting Symbols on process.env
1aa595e introduced a `throw` for accessing `Symbol` properties of
`process.env`. However, this breaks `util.inspect(process)` and
things like `Object.prototype.toString.call(process.env)`, so this
patch changes the behaviour for the getter to just always return
`undefined`.
@@ -2681,6 +2681,9 @@ static void ProcessTitleSetter(Local<Name> property,
static void EnvGetter(Local<Name> property,
const PropertyCallbackInfo<Value>& info) {
Isolate* isolate = info.GetIsolate();
if (property->IsSymbol()) {
return info.GetReturnValue().SetUndefined();

This comment has been minimized.

@bnoordhuis

bnoordhuis Nov 16, 2016

Member

Calling .SetUndefined() explicitly doesn't hurt but it's not strictly necessary as it's the default return value. We don't call it anywhere else.

@bnoordhuis

bnoordhuis Nov 16, 2016

Member

Calling .SetUndefined() explicitly doesn't hurt but it's not strictly necessary as it's the default return value. We don't call it anywhere else.

This comment has been minimized.

@addaleax

addaleax Nov 16, 2016

Member

@bnoordhuis I think making the return value explicit is more readable here, that’s all. If you prefer, I have no problem dropping it.

@addaleax

addaleax Nov 16, 2016

Member

@bnoordhuis I think making the return value explicit is more readable here, that’s all. If you prefer, I have no problem dropping it.

This comment has been minimized.

@bnoordhuis

bnoordhuis Nov 16, 2016

Member

It's fine.

@bnoordhuis

bnoordhuis Nov 16, 2016

Member

It's fine.

This comment has been minimized.

@sam-github

sam-github Nov 16, 2016

Member

Isn't this the C++ equivalent of function() { return undefined; }? I've no strong feelings, but explicit isn't necessarily more readable, IMHO.

@sam-github

sam-github Nov 16, 2016

Member

Isn't this the C++ equivalent of function() { return undefined; }? I've no strong feelings, but explicit isn't necessarily more readable, IMHO.

This comment has been minimized.

@addaleax

addaleax Nov 16, 2016

Member

Isn't this the C++ equivalent of function() { return undefined; }?

Yup.

I've no strong feelings, but explicit isn't necessarily more readable, IMHO.

I agree, but in this instance I think it makes sense.

@addaleax

addaleax Nov 16, 2016

Member

Isn't this the C++ equivalent of function() { return undefined; }?

Yup.

I've no strong feelings, but explicit isn't necessarily more readable, IMHO.

I agree, but in this instance I think it makes sense.

@targos

targos approved these changes Nov 16, 2016

@mhdawson

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Nov 18, 2016

Member

Marking as semver-major due to the change in error handling.

Member

jasnell commented Nov 18, 2016

Marking as semver-major due to the change in error handling.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 20, 2016

Member

@jasnell This partially undoes a not-yet-released semver-major change, I’m not sure that qualifies as semver-major on its own?

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

Member

addaleax commented Nov 20, 2016

@jasnell This partially undoes a not-yet-released semver-major change, I’m not sure that qualifies as semver-major on its own?

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

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 20, 2016

Member

Landed in 3295a7f, thanks for reviewing everyone!

Member

addaleax commented Nov 20, 2016

Landed in 3295a7f, thanks for reviewing everyone!

@addaleax addaleax closed this Nov 20, 2016

addaleax added a commit that referenced this pull request Nov 20, 2016

src: allow getting Symbols on process.env
1aa595e introduced a `throw` for accessing `Symbol` properties of
`process.env`. However, this breaks `util.inspect(process)` and
things like `Object.prototype.toString.call(process.env)`, so this
patch changes the behaviour for the getter to just always return
`undefined`.

Ref: #9446
Fixes: #9641
PR-URL: #9631
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@addaleax addaleax deleted the addaleax:util-inspect-process-env branch Nov 20, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Nov 20, 2016

Member

It's a bit of a grey area

On Sat, Nov 19, 2016 at 4:05 PM Anna Henningsen notifications@github.com
wrote:

@jasnell https://github.com/jasnell This partially undoes a
not-yet-released semver-major change, I’m not sure that qualifies as
semver-major on its own?

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


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9631 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eYKDlWH8AgY0gpoQ5rtZ38ViTs_tks5q_47NgaJpZM4KzQBC
.

Member

jasnell commented Nov 20, 2016

It's a bit of a grey area

On Sat, Nov 19, 2016 at 4:05 PM Anna Henningsen notifications@github.com
wrote:

@jasnell https://github.com/jasnell This partially undoes a
not-yet-released semver-major change, I’m not sure that qualifies as
semver-major on its own?

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


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9631 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eYKDlWH8AgY0gpoQ5rtZ38ViTs_tks5q_47NgaJpZM4KzQBC
.

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017

src: allow getting Symbols on process.env
1aa595e introduced a `throw` for accessing `Symbol` properties of
`process.env`. However, this breaks `util.inspect(process)` and
things like `Object.prototype.toString.call(process.env)`, so this
patch changes the behaviour for the getter to just always return
`undefined`.

Ref: nodejs#9446
Fixes: nodejs#9641
PR-URL: nodejs#9631
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment