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

src: clean up usage of __proto__ #5069

Closed
wants to merge 1 commit into from
Closed

Conversation

JacksonTian
Copy link
Contributor

Prefer using Object.setPrototypeOf() instead.

Prefer using Object.setPrototypeOf() instead.
@thefourtheye thefourtheye added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 4, 2016
@thefourtheye
Copy link
Contributor

Has v8 deprecated __proto__ yet? cc @nodejs/v8

@domenic
Copy link
Contributor

domenic commented Feb 4, 2016

The ES spec has "deprecated" __proto__ by putting it in annex B, but it will never be removed from engines. Deprecation is dumb on the web and never works, and the whole concept of ES deprecating them is dumb.

That said, I still think it's reasonable to do this replacement, because it's prettier. I'm surprised this is the only usage of __proto__ in the codebase though.

@mscdex
Copy link
Contributor

mscdex commented Feb 4, 2016

This may be a silly question, but do these two things do exactly the same thing internally, at least in v8? I'm thinking about if there would be any weird/unexpected changes in performance using one method over the other (as far as object property access goes, not the time spent actually setting the prototype during startup).

@thefourtheye
Copy link
Contributor

Oh I didn't know that ES6 made __proto__ an optional feature for non-browser hosts.

ECMAScript implementations are discouraged from implementing these features unless the implementation is part of a web browser or is required to run the same legacy ECMAScript code that web browsers encounter.

Looks like usage of __proto__ is okay for the time being then.

@indutny
Copy link
Member

indutny commented Feb 4, 2016

I wonder if we can actually inherit it from EventEmitter instead of changing prototype.

@thefourtheye
Copy link
Contributor

process object is created during the Environment creation right? Will we be able to load EventEmitter by then?

@indutny
Copy link
Member

indutny commented Feb 4, 2016

No, but I think we can instantiate it later, or make this object internal to some object inherited from EventEmitter.

@thefourtheye
Copy link
Contributor

Do you mean creation of process object need not happen at the time of Environment creation?

@indutny
Copy link
Member

indutny commented Feb 4, 2016

There is no hard requirement for this.

I'm just throwing out random ideas, it doesn't mean that they should be turn into code :) At least not immediately! ;)

@thefourtheye
Copy link
Contributor

Okay :-) I was just trying to understand your ideas better.

@targos
Copy link
Member

targos commented Feb 4, 2016

@JacksonTian could you add the corresponding rule to our linter (in a separate commit) ?
It would be http://eslint.org/docs/rules/no-proto.html

@jasnell
Copy link
Member

jasnell commented Feb 4, 2016

LGTM

@trevnorris
Copy link
Contributor

LGTM

@r-52
Copy link
Contributor

r-52 commented Feb 5, 2016

@JacksonTian
Copy link
Contributor Author

@targos I will do it. thanks.

jasnell pushed a commit that referenced this pull request Feb 7, 2016
Prefer using Object.setPrototypeOf() instead.

PR-URL: #5069
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Feb 7, 2016

Landed in 9aebb00

@jasnell jasnell closed this Feb 7, 2016
rvagg pushed a commit that referenced this pull request Feb 8, 2016
Prefer using Object.setPrototypeOf() instead.

PR-URL: #5069
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 9, 2016
Prefer using Object.setPrototypeOf() instead.

PR-URL: #5069
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Prefer using Object.setPrototypeOf() instead.

PR-URL: #5069
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@JacksonTian JacksonTian deleted the proto branch February 18, 2016 02:15
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Prefer using Object.setPrototypeOf() instead.

PR-URL: #5069
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Prefer using Object.setPrototypeOf() instead.

PR-URL: #5069
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Prefer using Object.setPrototypeOf() instead.

PR-URL: nodejs#5069
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants