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 invalid date output with util.inspect #6504

Closed
wants to merge 1 commit into from
Closed

Fix invalid date output with util.inspect #6504

wants to merge 1 commit into from

Conversation

rumkin
Copy link
Contributor

@rumkin rumkin commented May 1, 2016 •
edited
Loading

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

util.inspect produces default string value for invalid date instead of throwing RangeError.

@cjihrig cjihrig mentioned this pull request May 1, 2016
Closed
4 tasks
cjihrig
cjihrig reviewed May 1, 2016
View reviewed changes
test/parallel/test-util-inspect.js
@@ -14,6 +14,7 @@ assert.equal(util.inspect(null), 'null');
assert.equal(util.inspect(/foo(bar\n)?/gi), '/foo(bar\\n)?/gi');
assert.equal(util.inspect(new Date('Sun, 14 Feb 2010 11:48:40 GMT')),
new Date('2010-02-14T12:48:40+01:00').toISOString());
assert.equal(util.inspect(new Date('')), (new Date('')).toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use assert.strictEqual() here.

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 make this simply:

assert.equal(util.inspect(new Date('')), 'Invalid Date');

there's no reason to create the second date object.

@cjihrig
Copy link
Contributor

cjihrig commented May 1, 2016

It's not a big deal because whoever lands this can fix it up but:

  1. This should target the master branch, not v6.x.
  2. The commit message should adhere to https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit.

@mscdex mscdex added the util Issues and PRs related to the built-in util module. label May 1, 2016
@jasnell jasnell added error-handling semver-major PRs that contain breaking changes and should be released in the next major version. labels May 1, 2016
jasnell
jasnell reviewed May 1, 2016
View reviewed changes
lib/util.js
return ctx.stylize(value.toString(), 'date');
} else {
return ctx.stylize(Date.prototype.toISOString.call(value), 'date');
}
Copy link
Member

Choose a reason for hiding this comment

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

just a style nit so feel free to ignore...

return ctx.stylize(Number.isNaN(value.getTime()) ?
                     value.toString() :
                     Date.prototype.toISOString.call(value),
                   'date');

... would help eliminate some minor code duplication

@jasnell
Copy link
Member

jasnell commented May 1, 2016

semver-major because of the change in error handling (this eliminates a throw).

@jasnell
Copy link
Member

jasnell commented May 1, 2016

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

@addaleax
Copy link
Member

addaleax commented May 1, 2016 •
edited
Loading

This restores the pre-v6.0.0 behaviour and probably aligns much more with user expectations than the current behaviour, so I would actually say it’s a bugfix. Otherwise, this means that no object containing an invalid Date object could be printed using console.log or in the repl.

@cjihrig
Copy link
Contributor

cjihrig commented May 1, 2016

+1 to bug fix.

@jasnell
Copy link
Member

jasnell commented May 1, 2016

Works for me!

@jasnell jasnell removed the semver-major PRs that contain breaking changes and should be released in the next major version. label May 1, 2016
@rumkin
Copy link
Contributor Author

rumkin commented May 2, 2016

@cjihrig how should I fix branch and comment now in better way?

@cjihrig
Copy link
Contributor

cjihrig commented May 2, 2016

Github doesn't allow you to target a new branch once a PR is made. However, master and v6.x are so similar, that I don't think it should matter in this case.

To change your commit message, you can run git commit --amend, then force push to the branch you used for this PR (in this case, v6.x).

@Fishrock123 Fishrock123 changed the title Fix invalid date output with util.ispect Fix invalid date output with util.inspect May 2, 2016
@rumkin
Copy link
Contributor Author

rumkin commented May 3, 2016

@cjihrig Done

@cjihrig
Copy link
Contributor

cjihrig commented May 3, 2016

LGTM once the comment about using assert.strictEqual() is addressed. I'd probably change the commit subsystem to just util too.

@rumkin
util: fix invalid date output with util.inspect
f3496d3 
Prevent util.inspect of throwing on date object with invalid date value.
It changed to output result of toString method call.
@jasnell
Copy link
Member

jasnell commented May 6, 2016

@cjihrig ... PTAL
LGTM with the updates

Note to @rumkin : we don't get notified when new or updated commits are pushed so it can be easy to miss. Best bet is to add a comment after pushing the new commits so we get notified! :-)

@cjihrig
Copy link
Contributor

cjihrig commented May 6, 2016

LGTM. Thanks for the notification @jasnell

@rumkin
Copy link
Contributor Author

rumkin commented May 7, 2016

@cjihrig @jasnell Thanks for help with my PR.

@evanlucas
Copy link
Contributor

LGTM. Running CI one more time https://ci.nodejs.org/job/node-test-pull-request/2559/

@rumkin
Copy link
Contributor Author

rumkin commented May 16, 2016

@cjihrig what's next? Will PR be accepted?

@addaleax
Copy link
Member

@rumkin Thanks for commenting here again, sometimes things like this get a little lost.

This LGTM and I’m landing it now.

addaleax pushed a commit that referenced this pull request May 16, 2016
@rumkin @addaleax
util: fix invalid date output with util.inspect
9c33e0e 
Prevent util.inspect of throwing on date object with invalid date value.
It changed to output result of toString method call.

PR-URL: #6504
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax
Copy link
Member

Landed in 9c33e0e. Thanks!

@addaleax addaleax closed this May 16, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
@rumkin @evanlucas
util: fix invalid date output with util.inspect
6d1527b 
Prevent util.inspect of throwing on date object with invalid date value.
It changed to output result of toString method call.

PR-URL: #6504
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@evanlucas evanlucas mentioned this pull request May 17, 2016
Merged
@MylesBorins
Copy link
Contributor

@nodejs/lts is this something we want to backport?

@addaleax
Copy link
Member

addaleax commented Jun 2, 2016

@thealphanerd Just fyi, this was prompted by a change that came with the v8 changes in v6.0.0 and should be a no-op in the previous versions.

ljharb added a commit to ljharb/es-to-primitive that referenced this pull request Aug 27, 2017
@ljharb
[Tests] avoid util.inspect bug with new Date(NaN) on node v6.0 and …
01cebe3 
…v6.1.

nodejs/node#6504
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rumkin @cjihrig @jasnell @addaleax @evanlucas @MylesBorins @mscdex