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

[attrs] Don't rely on toJSON/string-parsing on DateAttr serialization #55

Merged
merged 1 commit into from Sep 26, 2018

Conversation

Projects
None yet
3 participants
@peterwmwong
Copy link
Contributor

peterwmwong commented Sep 26, 2018

Relying on Date.prototype.toJSON() and string manipulation is causing the incorrect date to be serialized...

screen shot 2018-09-26 at 11 44 30 am

@peterwmwong

This comment has been minimized.

Copy link
Contributor

peterwmwong commented Sep 26, 2018

Unfortunately, there doesn't seem to be a great way to test this. The builtin Date API doesn't allow us to set the user's timezone. For the specs running in node, the only way I know of would be to mess with TZ environment variable, but no good way during runtime I know of.

@cwitthaus

This comment has been minimized.

@peterwmwong

This comment has been minimized.

Copy link
Contributor

peterwmwong commented Sep 26, 2018

Oh @cwitthaus ....

@burrows

This comment has been minimized.

Copy link
Member

burrows commented Sep 26, 2018

LGTM.

if (date instanceof Date) {
return date.getFullYear() + '-'
+ format2Digits(date.getMonth() + 1, 2) + '-'
+ format2Digits(date.getDate(), 2);

This comment has been minimized.

@burrows

burrows Sep 26, 2018

Member

The second param to format2Digits isn't getting used.

This comment has been minimized.

@peterwmwong

peterwmwong Sep 26, 2018

Contributor

Gah, thanks fixed: f7dac6d

@peterwmwong peterwmwong force-pushed the fix-date-attr-serialization branch from a08a982 to 8d03225 Sep 26, 2018

@peterwmwong peterwmwong force-pushed the fix-date-attr-serialization branch from 8d03225 to f7dac6d Sep 26, 2018

@peterwmwong peterwmwong merged commit 8b189e8 into master Sep 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment