lib,src: make constants not inherit from Object#10458
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added c++ 
addaleax added lib / src 
lib/fs.js Outdated
There was a problem hiding this comment.
I think these have a small performance impact, maybe Object.prototype.hasOwnProperty.call is better for all of these?
There was a problem hiding this comment.
As constants inherit from null, will the performance implication still be applicable?
There was a problem hiding this comment.
Performance of the in operator has recently been optimized in V8 (in 5.2).
There was a problem hiding this comment.
@targos It's still quite slow though in master (V8 5.4). I recently swapped usage of in with a simpler undefined check and saw a ~50% improvement.
@thefourtheye I would just do an undefined check (constants.O_SYMLINK !== undefined) instead of in. It should still work fine.
There was a problem hiding this comment.
Done! I changed them to !== undefined.
targos commented Dec 26, 2016
What do we gain from this change? |
thefourtheye commented Dec 26, 2016
thefourtheyeforce-pushed the make-constants-inherit-from-null branch from 11a0b88 to 8638f31Compare
There was a problem hiding this comment.
Can you capitalize "make" please.
There was a problem hiding this comment.
Can you name this with camelCase please.
thefourtheyeforce-pushed the make-constants-inherit-from-null branch from 8638f31 to aaf7165Compare
mscdex commented Dec 27, 2016
Fishrock123 left a comment
Fishrock123 left a comment There was a problem hiding this comment.
LGTM for a major (if it works, ci)
mscdex commented Jan 3, 2017
LGTM |
jasnell commented Jan 3, 2017
I'm wondering if a similar semver-major change should not be made to the docs-only deprecated |
thefourtheye commented Jan 3, 2017
@jasnell Do you mean the object returned by |
thefourtheyeforce-pushed the make-constants-inherit-from-null branch from aaf7165 to 002ce78Compare
Make sure `constants` object and all the nested objects don't inherit from `Object.prototype` but from `null`.
thefourtheyeforce-pushed the make-constants-inherit-from-null branch from 002ce78 to 3a75d7cComparejasnell commented Jan 3, 2017
@thefourtheye ... yes, that's what I'm thinking. We could choose to leave it untouched but for consistency it may make sense |
thefourtheye commented Jan 4, 2017
@jasnell I just tried that and it affects graceful-fs, https://github.com/isaacs/node-graceful-fs/blob/v4.1.11/polyfills.js#L31 :'( |
jasnell commented Jan 4, 2017
Boo. Ok, I'd leave it as is then. |
thefourtheye commented Jan 15, 2017
cc @nodejs/ctc as this is a major change. |
thefourtheye commented Feb 13, 2017
Yet another CI Run to BUMP https://ci.nodejs.org/job/node-test-pull-request/6372/ |
jasnell commented Feb 13, 2017
@thefourtheye ... this should be good to go! |
thefourtheye commented Feb 13, 2017
@jasnell Thanks :-) I would be comfortable if I get a green CITGM as well, as this is a major change. I'll try to spend sometime this week to fix it. |
thefourtheye commented Feb 23, 2017
jasnell commented Mar 22, 2017
CITGM and CI both were good. Landing. |
Make sure `constants` object and all the nested objects don't inherit from `Object.prototype` but from `null`. PR-URL: #10458 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Brian White <[email protected]>
jasnell commented Mar 22, 2017
Landed in caf9ae7 |
8 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
lib src test
Description of change
Make sure
constantsobject and all the nested objects don't inheritfrom
Object.prototypebut fromnull.