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.

Sign up for GitHub

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

Jump to bottom

process: keep process prototype in inheritance chain #14715

Closed
wants to merge 1 commit into from
Closed

process: keep process prototype in inheritance chain #14715

wants to merge 1 commit into from

Conversation

MSLaguana
Copy link
Contributor

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 EventEmitters.

Fixes: #14699

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

process

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 9, 2017
vsemozhetbyt
vsemozhetbyt reviewed Aug 9, 2017
View reviewed changes
lib/internal/bootstrap_node.js Outdated
Object.setPrototypeOf(process, Object.create(EventEmitter.prototype, {
constructor: Object.getOwnPropertyDescriptor(origProcProto, 'constructor')
}));
Object.setPrototypeOf(Object.getPrototypeOf(process), EventEmitter.prototype);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @MSLaguana!
Sorry for the nits, but we have two ESLint complaints here.

  16:11  error  'origProcProto' is assigned a value but never used  no-unused-vars
  17:1   error  Line 17 exceeds the maximum line length of 80       max-len

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gah, that's what I get for writing the same thing on a few machines to test it all out. Fixed.

@mscdex mscdex added the process Issues and PRs related to the process subsystem. label Aug 9, 2017
@MSLaguana MSLaguana force-pushed the changeProcessInheritance branch from e43c580 to c3dc069 Compare August 9, 2017 16:28
@vsemozhetbyt
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/9575/

Trott
Trott previously requested changes Aug 10, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it should be possible to add a test for this. Maybe just assert.strictEqual(process.__proto__, process.__proto__.constructor.prototype); inside test/parallel/test-process-prototype.js?

@Trott
Copy link
Member

Trott commented Aug 10, 2017

Hi, @MSLaguana! Thanks for the PR and trying to introduce some more predictability/intuitiveness to Node.js.

@MSLaguana
process: keep process prototype in inheritance chain
a0b4a4a 
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
@MSLaguana MSLaguana force-pushed the changeProcessInheritance branch from c3dc069 to a0b4a4a Compare August 10, 2017 16:40
@MSLaguana
Copy link
Contributor Author

MSLaguana commented Aug 10, 2017 •
edited
Loading

@Trott fair point, added a test to make sure that process instanceof process.constructor along with the previous test that process.__proto__ instanceof EventEmitter.

@Trott Trott dismissed their stale review August 10, 2017 20:35

test added, dismissing request for test

@addaleax
Copy link
Member

Fresh CI: https://ci.nodejs.org/job/node-test-commit/11761/

This should be ready.

@addaleax
Copy link
Member

CI is good

Landed in cde272a, thanks for the PR! 🎉

@addaleax addaleax closed this Aug 14, 2017
addaleax pushed a commit that referenced this pull request Aug 14, 2017
@MSLaguana @addaleax
process: keep process prototype in inheritance chain
cde272a 
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 <[email protected]>
@MSLaguana MSLaguana deleted the changeProcessInheritance branch August 14, 2017 21:35
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@MSLaguana @MylesBorins
process: keep process prototype in inheritance chain
4d2aa16 
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 <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@MylesBorins
Copy link
Contributor

ping re: backport

@MSLaguana
Copy link
Contributor Author

Sorry for the delay. I don't think it is important to backport this, it was more for cleaning up (and to allow improvements in node-chakracore).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott left review comments

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Why does the global process object have its prototype replaced?
7 participants
@MSLaguana @vsemozhetbyt @Trott @addaleax @MylesBorins @mscdex @nodejs-github-bot