Skip to content
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

Why does the global process object have its prototype replaced? #14699

Closed
MSLaguana opened this issue Aug 8, 2017 · 4 comments
Closed

Why does the global process object have its prototype replaced? #14699

MSLaguana opened this issue Aug 8, 2017 · 4 comments
Labels
process Issues and PRs related to the process subsystem. question Issues that look for answers.

Comments

@MSLaguana
Copy link
Contributor

MSLaguana commented Aug 8, 2017

  • Version: All
  • Platform: All
  • Subsystem: process

I have been investigating some behaviors of node-chakracore, and I found some confusing code around the process object. In https://github.com/nodejs/node/blob/master/lib/internal/bootstrap_node.js#L17 the global process object instance has its __proto__ replaced with a fresh object that in turn has __proto__ of EventEmitter.prototype. That is, process.__proto__ !== process.__proto__.constructor.prototype and process.__proto__.__proto__ === EventEmitter.prototype.

What is the reason behind this, and why not simply set process.__proto__.constructor.prototype.__proto__ to be EventEmitter.prototype, preferably in the native code where process's constructor function is defined (although I haven't checked the order of instantiation of EventEmitter and process yet)?

The reason that I care about this is it is currently blocking an improvement in node-chakracore regarding how some objects toString() to something like [object process] rather than [object Object]. That improvement works for all cases except for the global process instance because it works by adding a Symbol.toStringTag to the object prototype, which is explicitly overridden for process.

@mscdex mscdex added process Issues and PRs related to the process subsystem. question Issues that look for answers. labels Aug 8, 2017
@bnoordhuis
Copy link
Member

Just so we are on the same page, you are essentially asking for this change?

diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js
index cf517cdcf2..efdb63b5e9 100644
--- a/lib/internal/bootstrap_node.js
+++ b/lib/internal/bootstrap_node.js
@@ -13,10 +13,8 @@
     const EventEmitter = NativeModule.require('events');
     process._eventsCount = 0;
 
-    const origProcProto = Object.getPrototypeOf(process);
-    Object.setPrototypeOf(process, Object.create(EventEmitter.prototype, {
-      constructor: Object.getOwnPropertyDescriptor(origProcProto, 'constructor')
-    }));
+    Object.setPrototypeOf(Object.getPrototypeOf(process).constructor.prototype,
+                          EventEmitter.prototype);
 
     EventEmitter.call(process);
 

To be honest, I don't know why we do what we do. process.constructor.name === 'process' and Object.prototype.toString.call(process) === '[object process]' either way.

@MSLaguana
Copy link
Contributor Author

Even simpler than that end result: Object.setPrototypeOf(Object.getPrototypeOf(process), EventEmitter.prototype)

Unless there is some underlying reason, then this would seem to be more intuitive.

Do note that just because process.constructor.name === 'process' that has no relation to process.toString() === '[object process]'; if you left off the constructor method on the copy that is currently created it would still hold true because of how the process is configured in native code. The way that node-chakracore is shimming the v8 approach is actually what I am trying to improve here.

@refack
Copy link
Contributor

refack commented Aug 9, 2017

@MSLaguana I dug a little and he're the story:
It was made to inherit EE but be considered of class process
6d70a4a
Then is was decided to just copy the ctor property instead of just the value
e0bc5a7

if you can make the tests pass, you can PR a change, and IMHO it'll get the proper discussion.

@MSLaguana
Copy link
Contributor Author

Thanks @refack, I'll give it a go. Just wanted to make sure there wasn't some subtle reason that I was missing.

MSLaguana added a commit to MSLaguana/node that referenced this issue Aug 10, 2017
The global `process` object had its prototype replaced with a
fresh object that had `EventEmitter.prototype` as its prototype.
With this change, the original `process.constructor.prototype` is
modified to have `EventEmitter.prototype` as its prototype, reflecting
that `process` objects are also `EventEmitter`s.

Fixes: nodejs#14699
MylesBorins pushed a commit that referenced this issue Sep 10, 2017
The global `process` object had its prototype replaced with a
fresh object that had `EventEmitter.prototype` as its prototype.
With this change, the original `process.constructor.prototype` is
modified to have `EventEmitter.prototype` as its prototype, reflecting
that `process` objects are also `EventEmitter`s.

Fixes: #14699
PR-URL: #14715
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. question Issues that look for answers.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants