src: allow getting Symbols on process.env#9631
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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`.
addaleax added c++ 
nodejs-github-bot added the c++ 
| const PropertyCallbackInfo<Value>& info){ | ||
| Isolate* isolate = info.GetIsolate(); | ||
| if (property->IsSymbol()){ | ||
| return info.GetReturnValue().SetUndefined(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@bnoordhuis I think making the return value explicit is more readable here, that’s all. If you prefer, I have no problem dropping it.
There was a problem hiding this comment.
Isn't this the C++ equivalent of function(){return undefined}? I've no strong feelings, but explicit isn't necessarily more readable, IMHO.
There was a problem hiding this comment.
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.
jasnell added the semver-major 
jasnell commented Nov 18, 2016
Marking as semver-major due to the change in error handling. |
addaleax commented Nov 20, 2016
@jasnell This partially undoes a not-yet-released |
addaleax commented Nov 20, 2016
Landed in 3295a7f, thanks for reviewing everyone! |
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: #9446Fixes: #9641 PR-URL: #9631 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
jasnell commented Nov 20, 2016
It's a bit of a grey area On Sat, Nov 19, 2016 at 4:05 PM Anna Henningsen [email protected]
|
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#9446Fixes: nodejs#9641 PR-URL: nodejs#9631 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
8 participants
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
process
Description of change
1aa595e introduced a
throwfor accessingSymbolproperties ofprocess.env. However, this breaksutil.inspect(process)and things likeObject.prototype.toString.call(process.env), so this patch changes the behaviour for the getter to just always returnundefined./cc @cjihrig