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

Weird problem when parsing dates: different result depending on the current day for the same string #98

Closed
jorgemanrubia opened this Issue Feb 1, 2012 · 12 comments

Comments

Projects
None yet
2 participants
@jorgemanrubia

jorgemanrubia commented Feb 1, 2012

Hi,

Today we experienced a very weird problem with suggar.js. During a period of time, we observed a general problem in the way the dates in our app worked. We debugged the problem, and realized that during the day "31st of January" sugar.js didn't parse dates correctly.

You can simulate this problem by changing the current day in your operating system.

today = 15 february (everything is fine, 2011-09... is parsed as september)

    > Date.create('2011-09-01T05:00:00Z').toString()
    "Thu Sep 01 2011 01:00:00 GMT-0400 (EDT)"

today = 30 february (2011-09... is parsed as october)

    > Date.create('2011-09-01T05:00:00Z').toString()
    "Sat Oct 01 2011 01:00:00 GMT-0400 (EDT)"

today = 31 february (2011-09... is parsed as august)

    > Date.create('2011-09-01T05:00:00Z').toString()
    "Mon Aug 01 2011 01:00:00 GMT-0400 (EDT)"

We updated to the last version of sugar.js but the problem persists.

Any help with this issue would be appreciated.

Regards

@andrewplummer

This comment has been minimized.

Owner

andrewplummer commented Feb 2, 2012

Excellent catch! I have a fix for this, will go out with the next release.

This was actually quite a tricky issue... essentially setting UTC values, specifically hours and more specific, has the potential to traverse into other days/months/years. Here's an example:

var a = new Date(2011, 8, 30, 22, 1, 31);
a.setUTCHours(4);
console.log(a);

This will end up October 1st for me. The implications here are subtle but in any case the solution was to preemptively reset the time in cases when UTC dates are about to be set.

@jorgemanrubia

This comment has been minimized.

jorgemanrubia commented Feb 2, 2012

Hi Andrew,

Thank you so much for the quick response and explanation. Really looking forward to that release.

I have also checked that this problem also happens even if you don't use Sugar.js. Using new Date() instead of Date.create() was also failing in the same way I described (even without loading sugar.js).

Is the new release going to address this problem when using "Date.create()" with an UTC string? I just want to confirm, because I was thinking in changing the format for the dates we were managing. It's not something trivial and I'd rather wait to have that fix. Just want to be sure.

Thanks again.

Regards

@jorgemanrubia

This comment has been minimized.

jorgemanrubia commented Feb 2, 2012

Also, could you tell me when do you plan to release the fixed version? I'd understand if you don't know it yet, but it would be good for our planning if you know.

Thanks again

@andrewplummer

This comment has been minimized.

Owner

andrewplummer commented Feb 2, 2012

OK I can't figure this out as I'm struggling with changing my system time on stupid Ubuntu 11 and VirtualBoxes (harder than you'd think :) but it seems unlikely that the system without Sugar would have the same error unless there was an identical internal bug... hard to imagine though.

Yes the fix will address using Date.create on UTC strings. I'll release the fix as soon as I can... within this week if possible. For now you can probably workaround (I think..haven't tried this yet though) by stripping off the "Z", parsing the date, then calling toUTC on it, as that will take it through an entirely different code path... no guarantees, but try it out.

@andrewplummer

This comment has been minimized.

Owner

andrewplummer commented Feb 2, 2012

OK finally tried it... looks like the method above should work for now. Don't worry, will make it a point to release soon though.

@jorgemanrubia

This comment has been minimized.

jorgemanrubia commented Feb 2, 2012

Awesome Andrew. Thank you so much

@andrewplummer

This comment has been minimized.

Owner

andrewplummer commented Feb 2, 2012

FWIW the fix solved about 30 unit test failures that were occurring on January 31st in CST so totally +1

andrewplummer added a commit that referenced this issue Feb 2, 2012

@jorgemanrubia

This comment has been minimized.

jorgemanrubia commented Feb 2, 2012

That was impressive Andrew. What a response time! I also love to know the lib is backed up with a great test suite.

Thanks

@jorgemanrubia

This comment has been minimized.

jorgemanrubia commented Feb 3, 2012

Hi Andrew,

Do you plan to release a new version containing this fix? Could I take it from the edge build? Or do you recommend me to wait until released? It is for an app currently in production so I value stability. Since the patch is a single line I am also considering to apply it manually to the build of sugar we are using for now.

@andrewplummer

This comment has been minimized.

Owner

andrewplummer commented Feb 3, 2012

If you need it out before the next few days (lets say 3) then I would say cherry pick it manually (some more stuff came in that bears new testing). Otherwise I hope to have the next update out within 3 days.

@andrewplummer

This comment has been minimized.

Owner

andrewplummer commented Feb 7, 2012

v1.2.2 is out now which fixes this issue!

@jorgemanrubia

This comment has been minimized.

jorgemanrubia commented Feb 7, 2012

Thanks for telling Andrew. We applied the fix manually but we'll update to this new version very soon.

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