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

url: use util's getConstructorOf #12526

Closed
wants to merge 2 commits into from

Conversation

@TimothyGu
Copy link
Member

commented Apr 20, 2017

Reduces code duplication.

Checklist
Affected core subsystem(s)

url, util

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

@@ -410,7 +394,7 @@ function formatValue(ctx, value, recurseTimes) {
});
}

var constructor = getConstructorOf(value);
var constructor = internalUtil.getConstructorOf(value);

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 20, 2017

Contributor

Can we extract this at the top of the file so we're not referencing internalUtil each time? Until lib/internal/util.js gets refactored to use the "new" module.exports = { ... } format, the property lookup could be slow.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 22, 2017

Author Member

We have done just that in 9077b48 / #11404.

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 24, 2017

Member

Btw, internal/util has now been refactored to use the module.exports = { ... } format.


exports.getConstructorOf = function getConstructorOf(obj) {
while (obj) {
var descriptor = Object.getOwnPropertyDescriptor(obj, 'constructor');

This comment has been minimized.

Copy link
@jseijas

jseijas Apr 20, 2017

Contributor

The performance of Object.getOwnPropertyDescriptor is about 17 times slower than Object.prototype.hasOwnProperty.call. Perhaps the code from getEligibleConstructor can be used instead.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 22, 2017

Author Member

That would be a breaking change for a class that defines constructor as accessors.

This comment has been minimized.

Copy link
@jseijas

jseijas Apr 22, 2017

Contributor

I've been asking myself in which case I'll need to do that. And I don't find the answer. Can you provide me some example of a case where you really want to do that?

This comment has been minimized.

Copy link
@jseijas

jseijas Apr 22, 2017

Contributor

I'll answer myself: even if I don't found any case, it should be a breaking change if someone does it that way.
But remember that URL constructor always has been using hasOwnProperty and now it will use getOwnPropetyDescriptor, so it means that a -1700% performance will happen. I'm ok with that but perhaps someone is not.

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 22, 2017

Member

@TimothyGu the code currently explicitly skips over any get constructor() {} - previously in url, it only used hasOwnProperty, so as-is, this is a breaking change for URL. it's only in lib/util that this is keeping the behavior the same.

In other words, any deduplication of this code seems to be a breaking change in one place or the other.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 22, 2017

Author Member

@ljharb, the URL change that added hasOwnProperty has not been released yet. In fact, the WHATWG URL API is still in experimental phase, and so compatibility on the URL side is not an issue.

@jseijas the URL constructor is not using this function, only the custom inspection function (which is not performance-sensitive at all) is using it.

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2017

Landed in b2870a4...061c5da.

@TimothyGu TimothyGu closed this Apr 24, 2017

@TimothyGu TimothyGu deleted the TimothyGu:util-inspect-constr branch Apr 24, 2017

TimothyGu added a commit that referenced this pull request Apr 24, 2017
util: move getConstructorOf() to internal
PR-URL: #12526
Reviewed-By: James M Snell <jasnell@gmail.com>
TimothyGu added a commit that referenced this pull request Apr 24, 2017
url: use internal/util's getConstructorOf
PR-URL: #12526
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell referenced this pull request May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6 participants
You can’t perform that action at this time.