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

console: coerce label to string in console.time() #14643

Closed
wants to merge 2 commits into from
Closed

console: coerce label to string in console.time() #14643

wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 5, 2017

Per the console spec, the label in console.time() is a string. Change is made for consistency with browser behavior.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

console

@jasnell
console: coerce label to string in console.time()
d2ad13c 
Per the console spec, the label in console.time() is a string.
@nodejs-github-bot nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Aug 5, 2017
@jasnell
console: use default value for console.time()
7488fde 
Per the console spec, the default value of label is `'default'`.
@refack refack mentioned this pull request Aug 5, 2017
Closed
3 tasks
refack
refack approved these changes Aug 5, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

1 nit

lib/console.js
@@ -139,12 +139,14 @@ Console.prototype.dir = function dir(object, options) {
};


Console.prototype.time = function time(label) {
Console.prototype.time = function time(label = 'default') {
label = `${label}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply String(label)?

Copy link
Member

Choose a reason for hiding this comment

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

@refack I guess it's because String(Symbol('s')) does not throw.

Copy link
Contributor

Choose a reason for hiding this comment

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

What a weird language 😖

Copy link
Contributor

@refack refack Aug 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Found the piece I was missing, the console IDL - https://console.spec.whatwg.org/#console-namespace
@jasnell could you find a suitable place to reference that in the code?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, IDL rules are weird. Before ES6 you could also use label + '' but not anymore due to some @@toPrimitive semantics that wouldn't work with Dates.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can write a line of comment to explain this? Or maybe next time who see it will be confused again.

refack
refack reviewed Aug 5, 2017
View reviewed changes
lib/console.js
@@ -139,12 +139,14 @@ Console.prototype.dir = function dir(object, options) {
};


Console.prototype.time = function time(label) {
Console.prototype.time = function time(label = 'default') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is okay because of the method binding in L62?

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean? It only binds the this value. And for the strictest compliance method.length === 0 because the IDL specifies label as an optional parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would have changed the length unless it was bound, which some (@jasnell) consider semver-major.
It does change console.__proto__.time.length a.k.a. console.Console.prototype.time.length.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW I do think this is semver-major, but because of the default change not .length.

XadillaX
XadillaX approved these changes Aug 6, 2017
View reviewed changes
lib/console.js
@@ -139,12 +139,14 @@ Console.prototype.dir = function dir(object, options) {
};


Console.prototype.time = function time(label) {
Console.prototype.time = function time(label = 'default') {
label = `${label}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can write a line of comment to explain this? Or maybe next time who see it will be confused again.

tniessen
tniessen approved these changes Aug 6, 2017
View reviewed changes
lib/console.js
@@ -139,12 +139,14 @@ Console.prototype.dir = function dir(object, options) {
};


Console.prototype.time = function time(label) {
Console.prototype.time = function time(label = 'default') {
label = `${label}`;
this._times.set(label, process.hrtime());
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I would just use the template literal in this line and spare the assignment above.

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 8, 2017
@jasnell
Copy link
Member Author

jasnell commented Aug 8, 2017

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

@jasnell
Copy link
Member Author

jasnell commented Aug 8, 2017

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

jasnell added a commit that referenced this pull request Aug 8, 2017
@jasnell
console: coerce label to string in console.time()
4da8b99 
Per the console spec, the label in console.time() is a string.
Per the console spec, the default value of label is `'default'`.

PR-URL: #14643
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Aug 8, 2017

Landed in 4da8b99

@jasnell jasnell closed this Aug 8, 2017
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
nikniv added a commit to nikniv/node that referenced this pull request Nov 28, 2017
@nikniv
test: expanded assertions for console.timeEnd() output
a5ac0e3 
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve nodejs#14544 but will
not address the whole thing.

Refs: nodejs#14643
@nikniv nikniv mentioned this pull request Nov 28, 2017
Closed
3 tasks
addaleax pushed a commit that referenced this pull request Dec 1, 2017
@nikniv @addaleax
test: expanded assertions for console.timeEnd() output
5282f96 
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve #14544 but will
not address the whole thing.

PR-URL: #17368
Refs: #14643
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@nikniv @MylesBorins
test: expanded assertions for console.timeEnd() output
efde4b7 
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve #14544 but will
not address the whole thing.

PR-URL: #17368
Refs: #14643
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@nikniv @MylesBorins
test: expanded assertions for console.timeEnd() output
ee1c95f 
Added assertions to verify that console.time() coerces labels to
strings correctly, by comparing against the expected output values of
console.timeEnd().

This helps resolve #14544 but will
not address the whole thing.

PR-URL: #17368
Refs: #14643
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@lpinca lpinca lpinca left review comments

@TimothyGu TimothyGu TimothyGu approved these changes

@cjihrig cjihrig cjihrig approved these changes

@XadillaX XadillaX XadillaX approved these changes

@tniessen tniessen tniessen approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
console Issues and PRs related to the console subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jasnell @refack @lpinca @TimothyGu @cjihrig @XadillaX @tniessen @BridgeAR @nodejs-github-bot