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

fix throwing RangeError on invalid date output #6469

Closed
wants to merge 2 commits into from
Closed

fix throwing RangeError on invalid date output #6469

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7cda526
fix throwing RangeError on invalid date output
rumkin Apr 29, 2016
b230617
fix code style
rumkin Apr 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,11 @@ function formatValue(ctx, value, recurseTimes) {
return ctx.stylize(RegExp.prototype.toString.call(value), 'regexp');
}
if (isDate(value)) {
return ctx.stylize(Date.prototype.toISOString.call(value), 'date');
if (value.toString() === 'Invalid Date') {
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, checking isNaN(value) should also work here since Date.prototype.valueOf() will return NaN when the date is invalid.

Copy link
Member

Choose a reason for hiding this comment

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

If that’s changed to isNaN(value), I’d like to see a comment explaining more or less exactly what @jasnell just wrote. :)

Copy link
Member

Choose a reason for hiding this comment

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

:-) not saying that it needs to be changed, only that it should work ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Number.isNaN(value.valueOf()) seems like the better way to go to me, even if it does require a code comment.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather see value.getTime() and compare that to an invalid date's getTime() that way it's obvious and no comment is required.

const invalidDate = Date.parse("invalid-date-string");
...
// later on
 if (isDate(value)) {
    if(Object.is(value.getTime(), invalidDate.getTime()) {
    //...

Copy link
Member

Choose a reason for hiding this comment

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

Insofar that it matters, the way V8 implements the Date object, getTime() is faster than valueOf() is faster than toString(). The first one is a field lookup, the others are calls into the runtime, with toString() polluting the date cache and creating a new string every time.

I'd write it as Number.isNaN(value.getTime()) or Object.is(value.getTime(), NaN) to avoid keeping around an extra object, though.

Copy link
Member

Choose a reason for hiding this comment

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

Works for me.
On Apr 30, 2016 3:05 AM, "Ben Noordhuis" notifications@github.com wrote:

In lib/util.js
#6469 (comment):

@@ -304,7 +304,11 @@ function formatValue(ctx, value, recurseTimes) {
return ctx.stylize(RegExp.prototype.toString.call(value), 'regexp');
}
if (isDate(value)) {

  •  return ctx.stylize(Date.prototype.toISOString.call(value), 'date');

  •  if (value.toString() === 'Invalid Date') {

Insofar that it matters, the way V8 implements the Date object, getTime()
is faster than valueOf() is faster than toString(). The first one is a
field lookup, the others are calls into the runtime, with toString()
polluting the date cache and creating a new string every time.

I'd write it as Number.isNaN(value.getTime()) or Object.is(value.getTime(),
NaN) to avoid keeping around an extra object, though.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6469/files/b23061713ea4c52d1280ca5f5d015aa92a62d529#r61667758

Copy link
Member

Choose a reason for hiding this comment

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

@bnoordhuis well, it means keeping an extra date object once. It can be kept in a utility and used everywhere invalid dates are needed :P

I'm fine with Number.isNaN(value.getTime(), NaN) although it's not obvious to readers who are not familiar with this obscure detail.

return ctx.stylize('Invalid Date', 'date');
} else {
return ctx.stylize(Date.prototype.toISOString.call(value), 'date');
}
}
if (isError(value)) {
return formatError(value);
Expand Down