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

console: allow Object.prototype fields as labels #563

Closed
wants to merge 1 commit into from
Closed

console: allow Object.prototype fields as labels #563

wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 23, 2015

Console.prototype.timeEnd() returns NaN if the timer label corresponds to a property on Object.prototype. This commit uses Object.create(null) to construct the _times object. See nodejs/node-v0.x-archive#9069

@vkurchatkin
Copy link
Contributor

LGTM!


// Verify that Object.prototype properties can be used as labels
assert.doesNotThrow(function () {
console.time('__proto__');
Copy link
Contributor

Choose a reason for hiding this comment

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

did this actually throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It returns NaN like I noted in the commit log :-/. Will update the test.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 25, 2015

@vkurchatkin updated the test. PTAL

@vkurchatkin
Copy link
Contributor

LGTM

if (!time) {
throw new Error('No such label: ' + label);
}
var duration = Date.now() - time;
let duration = Date.now() - time;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm generally -0 on replacing var with let unless there's a distinct need for block-scoping.

Edit: This is a non-blocking nit. It would make me feel better if these remained vars, though.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 26, 2015

@chrisdickinson changed the lets back to vars.

@vkurchatkin
Copy link
Contributor

I'll just leave it here: http://www.reddit.com/r/javascript/comments/1sgsmj/let_var_die/
Personally, I think let should be used everywhere instead of var

@Fishrock123
Copy link
Contributor

If we are going to switch, I think it most reasonable to only change lines that we are already editing.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 26, 2015

@vkurchatkin I'm in the camp of being as restrictive as possible, which I think let does a better job of than var.

@pluma
Copy link

pluma commented Jan 26, 2015

The test should still fail for __proto__:

console._times = Object.create(null);
console.time('__proto__'); // => console._times.__proto__ = Date.now();
console.timeEnd('__proto__'); // throws
console._times.__proto__ === null; // true

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 26, 2015

@pluma seems to work fine with the latest iojs code.

@pluma
Copy link

pluma commented Jan 26, 2015

@cjihrig Interesting. Is this related to the newer version of V8 being used?

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 26, 2015

@pluma that would be my guess - I see the same behavior in the Chrome console.

@pluma
Copy link

pluma commented Jan 26, 2015

Hm, odd. I guess the semantics of __proto__ Object.create(null) have changed in V8, then. Go ahead :shipit:

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 27, 2015

Is anyone opposed to this landing? Another alternative would be to use a Map because ES6.

@rvagg
Copy link
Member

rvagg commented Jan 27, 2015

+1 for landing, and also +1 for experimenting with Map because if there's somewhere we can afford to pay any performance penalty that may come with them (I'm assuming this of most new ES6 features) then it's in console.

Console.prototype.timeEnd() returns NaN if the timer label
corresponds to a property on Object.prototype. This commit
uses a Map to construct the _times object.
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 27, 2015

@rvagg updated to use a Map

@rvagg
Copy link
Member

rvagg commented Jan 27, 2015

love it!

@@ -21,7 +21,7 @@ function Console(stdout, stderr) {
Object.defineProperty(this, '_stdout', prop);
prop.value = stderr;
Object.defineProperty(this, '_stderr', prop);
prop.value = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better (perf-wise) to just replace this with { __proto__: null } or Object.create(null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@caitp I literally just changed from Object.create(null) to a Map. For future reference, if there ends up being a problem, it will be a very simple fix. I think I like @rvagg's idea to experiment with a Map here since performance is not crucial.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough, try it out

cjihrig added a commit that referenced this pull request Jan 27, 2015
Console.prototype.timeEnd() returns NaN if the timer label
corresponds to a property on Object.prototype. This commit
uses a Map to construct the _times object.

Fixes: nodejs/node-v0.x-archive#9069
PR-URL: #563
Reviewed-By: Vladimir Kurchatkin <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 27, 2015

Landed in 3cbb5cd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants