Fix pretty format failing when printing invalid dates

[huafu]

Summary

As suggested by @SimenB in #6631, this allow pretty format (snapshot serializer) to print invalid date objects instead of failing.

before:

expect(new Date(NaN)).toMatchSnapshot()
// will throw a RangeError: Invalid time value
//        at Date.toISOString (<anonymous>)

after:
The snapshot will contain Date { NaN } as of the date value.

Test plan

A test has been added.

[huafu]

Not sure if I should add this, but for GitHub to auto close the issue: this PR will close #6631

[codecov-io]

Codecov Report

Merging #6635 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #6635   +/-   ##
=======================================
  Coverage   63.57%   63.57%           
=======================================
  Files         235      235           
  Lines        8992     8992           
  Branches        3        4    +1     
=======================================
  Hits         5717     5717           
  Misses       3274     3274           
  Partials        1        1

Impacted Files	Coverage Δ
packages/pretty-format/src/index.js	96.37% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c61baa...cea8b23. Read the comment docs.

[huafu]

All the checks were successful with the previous commit, I've just moved around something in the changelog (I did place it at the wrong position in the first place) and now Travis check is failing. Can someone re-start the job in TravisCI please? or tell me what in 8cf31da is making travis to fail :-D

[huafu]

...ah yeah, GitHub status saying We're investigating reports of intermittent connection failures.... Should I force push or wait for someone to restart just the travis check?

[SimenB]

Don't mind Travis 🙂

[rickhanlonii]

Good stuff, is there something better we can use than "invalid-date"

Some ideas:

• Date(invalid)
• [Date invalid]
• Date<invalid>

[huafu]

@SimenB thanks for reviewing.
@rickhanlonii I chose invalid-date because I thought it was going well between true, false, null, undefined. Tho an invalid date is an object and not a primitive after all. And objects, sets, maps, ... are ClassName {...}.
So if it has to not be invalid-date, I'd rather go to something like one of those 2, to stay as close as possible to other kind of object instance values:

1. Date { Invalid }
2. Date { NaN }

I actually like the second one, since an invalid date's primitive value is NaN. Lemme know what I should change to and I'll do it.

[huafu]

...one thing tho, we have to remember that valid dates do not have Date anywhere around them:

  "date1": 2018-01-01T00:00:00.000Z
  "date2": [Date Invalid]
// or
  "date1": 2018-01-01T00:00:00.000Z
  "date2": Date(invalid)
// or
  "date1": 2018-01-01T00:00:00.000Z
  "date2": Date { NaN }
// or
  "date1": 2018-01-01T00:00:00.000Z
  "date2": invalid-date

[rickhanlonii]

My thinking is that "invalid-date" looks like a string and we pretty-print things other types as:

• Map { ... }
• Set { ... }
• Arguments [ ... ]
• DomNode [ ... ]

So it would make sense that we include the type for the invalid date:

• Date invalid-date
• Date { invalid }
• Date [invalid]

Check out the way we pretty print other types and try to stay consistent. One issue with the current implementation would be the diff for a test like:

test('example', () => {
  expect(Date(NaN)).toEqual('invalid-date');
});

Would fail and have a visually indistinguishable diff

[huafu]

@rickhanlonii did you miss my first reply to your message? ;-)

invalid-date looks like a string

I thought that too at first, but strings are surrounded with double quotes.

Anyway, as I said in my first reply, what about Date { NaN }? I like Date { invalid } but again NaN is the primitive value of invalid dates, so why not using it as well?

So you confirm, I guess it's either:

1. Date { invalid } or
2. Date { NaN }

I'm ready to change it now as soon as I got your choice ;-)

[rickhanlonii]

Oh shoot I missed that, good call out

I like Date { NaN } but both are good with me 🤙

[huafu]

Any ETA on when this will be merged @SimenB ? (or should I not worry about resolving conflicts in CHANGELOG.md anymore 🤣 ?)

[SimenB]

We can take care of a merge conflict if needed when we merge, no need for you to keep updating it 🙂

[SimenB]

@rickhanlonii does this LGTY? If so, we can probably merge it 🙂

[SimenB]

@rickhanlonii pingeling 😀

[rickhanlonii]

LGTM!

[SimenB]

We could also keep around Date.prototype.toString and fall back to that one. It prints Invalid Date.

const toStringValue = toString.call(val);

return toStringValue === 'Invalid Date' ? toStringValue : toISOString.call(val);

console.log(new Date(Infinity)) // 'Invalid Date'
console.log(new Date(NaN)) // 'Invalid Date'

EDIT: Or we can do Date { Invalid }.

return toString.call(val) === 'Invalid Date'
  ? 'Date { Invalid }'
  : toISOString.call(val);

[huafu]

too late :-p
anyway, I do not like testing over a "human" string. While an invalid date will always cast to the wonderful JavaScript number... not a number 🤣

[SimenB]

Hah, too late 😀

Feel free to send a new PR with this change if you think it makes sense

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.