fix throwing RangeError on invalid date output

[rumkin]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

Description of change

Invalid date value throws RangeError on output with console.log or util.inspect.

[addaleax]

Thank you! Could you maybe also write a test for this? The CONTRIBUTING.md contains some info on that.

[cjihrig]

Does this pass make lint? The else should go on the previous line with the closing curly brace. Actually, core style is to leave off the braces all together in cases like this.

[addaleax]

@rumkin Thanks for the changes! We don’t get notified when commits are added to PRs, so feel free to write a short message here whenever you do to let others know.

Have you had any luck writing a test? If you don’t have the time for that, that’s fine and can happen separately, I think.

[jasnell]

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

[addaleax]

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

[jasnell]

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

[cjihrig]

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

[benjamingr]

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()) {
    //...

[bnoordhuis]

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.

[jasnell]

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

[benjamingr]

@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.

[cjihrig]

@rumkin if you're looking for the right place to add a test,

node/test/parallel/test-util-inspect.js

Lines 15 to 17 in 706778a

	assert.equal(util.inspect(new Date('Sun, 14 Feb 2010 11:48:40 GMT')),
	new Date('2010-02-14T12:48:40+01:00').toISOString());

, seems appropriate :-)

[cjihrig]

Closing in favor of #6504

[rumkin]

I removed pull request repo accidentally. This pull request is duplicate with changes by @bnoordhuis and tests.

Thanks.