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.

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

Fix toJSON casting of invalid moment #2887

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@wraithgar
Contributor

wraithgar commented Jan 11, 2016

This is an attempt at solving issue #2886.

Invalid moments were casting as 'null' (the string)
Now they are properly casting as null (the literal)

Fix toJSON casting of invalid moment (#2886)
Invalid moments were casting as 'null' (the string)
Now they are properly casting as null (the literal)
@mj1856

View changes

src/lib/moment/to-type.js Outdated
@@ -29,6 +29,6 @@ export function toObject () {
}
export function toJSON () {
// JSON.stringify(new Date(NaN)) === 'null'
return this.isValid() ? this.toISOString() : 'null';
// JSON.stringify(new Date(NaN)) === null

This comment has been minimized.

@mj1856

mj1856 Jan 12, 2016

Member

The comment should just show new Date(NaN).toJSON() === null. It's the JSON.stringify part that is confusing the issue.

This comment has been minimized.

@wraithgar

wraithgar Jan 12, 2016

Contributor

Updated, thanks for the reminder, didn't think that one through enough. Fixed in commit dafaba3

This comment has been minimized.

@mj1856

mj1856 Jan 12, 2016

Member

Thanks.

@mj1856 mj1856 added this to the 2.12.0 milestone Jan 12, 2016

@mj1856

This comment has been minimized.

Member

mj1856 commented Jan 12, 2016

LGTM. Staging for 2.12.0 (unless @ichernev decides to do a 2.11.2).

Thanks for your contribution!

@mj1856 mj1856 added the Bug label Jan 12, 2016

@mj1856 mj1856 removed the Bug label Jan 12, 2016

@mj1856

This comment has been minimized.

Member

mj1856 commented Jan 12, 2016

(seems silly to label a bug fix with "bug") :)

@icambron

This comment has been minimized.

Member

icambron commented Jan 15, 2016

Do we know why it was made a string in the first place? Seems like a really explicit decision in d4a1694

@mj1856

This comment has been minimized.

Member

mj1856 commented Jan 16, 2016

@icambron - The comment leads me to believe there was a misunderstanding between toJSON and JSON.stringify. Consider:

JSON.stringify(NaN)    // "null"
JSON.stringify(null)   // "null"
JSON.stringify('null') // ""null""

JSON.stringify(new Date(NaN)) // "null"
new Date(NaN).toJSON() // null

Our implementation should match the Date object's toJSON implementation

@icambron

This comment has been minimized.

Member

icambron commented Jan 16, 2016

👍

@ichernev

This comment has been minimized.

Contributor

ichernev commented Mar 6, 2016

Merged in 2255756

@ichernev ichernev closed this Mar 6, 2016

ichernev added a commit that referenced this pull request Mar 6, 2016

Merge pull request #2887 from wraithgar:invalid_null_date
Fix toJSON casting of invalid moment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment