-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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
constants: freeze the constants object #19813
Conversation
Constants ought to be constant. The primary goal of this commit is to make constants exposed in require('constants') immutable, as they were prior to node@7.0.0, and as the constants exposed on fs.constants, crypto.constants, etc. are. Since this is implemented by using Object.freeze, it also has the side effect of making the entire exports of require('constants') immutable, so no new constants can be defined on the object in userland.
(Will need a CITGM run, of course.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if there are no horrible ecosystem repercussions.
Does a frozen object still cause performance issues when accessing properties? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it seems like a nice idea, I think I am -1 on it as we don't freeze other parts of our API and I don't think the inconsistency is a good thing.
Also, I'm not sure I understand the comment about constants being frozen prior to node v7. I'm able to write to the constants object just fine prior to v7. I can also overwrite existing property values in node v6.x (likely due to the use of Object.assign()
in lib/constants.js).
@mscdex Right, sorry, the change (to using Prior to dcccbfd, constants in
This is because prior to dcccbfd, Line 1116 in 091abb3
Which makes the properties immutable: Lines 233 to 234 in 091abb3
So this is a return to the original behavior, prior to v6.3.0. I used |
@mscdex Ping. Note also that |
Taking this off the 10.0.0 milestone, the deadline has passed anyway … and ping @mscdex re: the above |
Dismissing review due to missing response. PTAL
@nodejs/tsc PTAL. I dismissed the review from @mscdex |
I am going to land this in ~ 48 hours if there is no objection from anyone until then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Landed in 90e8f79 🎉 |
Constants ought to be constant. The primary goal of this commit is to make constants exposed in require('constants') immutable, as they were prior to node@7.0.0, and as the constants exposed on fs.constants, crypto.constants, etc. are. Since this is implemented by using Object.freeze, it also has the side effect of making the entire exports of require('constants') immutable, so no new constants can be defined on the object in userland. PR-URL: nodejs#19813 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Constants ought to be constant. The primary goal of this commit is to
make constants exposed in require('constants') immutable, as they were
prior to node@7.0.0, and as the constants exposed on fs.constants,
crypto.constants, etc. are.
Since this is implemented by using Object.freeze, it also has the side
effect of making the entire exports of require('constants') immutable,
so no new constants can be defined on the object in userland.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes