-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: disallow constructor behaviour for native methods #26700
Conversation
Disallow constructor behaviour and setting up prototypes for native methods that are not constructors (i.e. make them behave like ES6 class methods).
@@ -42,3 +42,11 @@ const err = { | |||
}; | |||
common.expectsError(function() { process.chdir({}); }, err); | |||
common.expectsError(function() { process.chdir(); }, err); | |||
|
|||
// Check that our built-in methods do not have a prototype/constructor behaviour | |||
// if they don't need to. This could be tested for any of our C++ methods. |
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.
This could be tested for any of our C++ methods.
Maybe it doesn't matter, but the one picked here means this isn't tested in Worker threads.
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.
Yeah, I’d say it doesn’t matter – these things are pretty independent, and I’ve chosen something that is likely to stay a directly C++-backed method for a long time. I’m happy to take other suggestions, though.
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.
Seems like this conflicted with #27224 😄
Nice, but what about @joyeecheung's assertion:
That's why I didn't do it in #26096, I felt like it should be firstly investigated if some modules rely on some methods/functions being constructors. |
@Hakerh400 We can run our ecosystem checking tool on this, but generally, this should be safe to do. None of the methods affected by this are likely to be used as constructors, and I’m not concerned that there could be significant ecosystem usage of this. Plus, this won’t be released until Node 12. |
Landed in 1935625 |
Disallow constructor behaviour and setting up prototypes for native methods that are not constructors (i.e. make them behave like ES6 class methods). PR-URL: #26700 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Disallow constructor behaviour and setting up prototypes
for native methods that are not constructors (i.e. make
them behave like ES6 class methods).
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes