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.

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

Fixes for non-compliant Date, multiple browsers. #360

Merged
merged 1 commit into from
Dec 27, 2015

Conversation

Xotic750
Copy link
Contributor

#240

Add extra tests to check string and other values.

This highlights differences between environments, i.e. strings. It depends on how far you want to take fixes/normalisation?

A version of eslint errors with theses changes, but not on any code that has been added.

  1405:19   warning  This function has too many statements (45). Maximum allowed is 30   max-statements
  1536:33   warning  This function has too many statements (33). Maximum allowed is 30   max-statements
  1556:13   error    use `string = String(string)` instead                               no-implicit-coercion
  1598:21   warning  Unary operator '++' used                                            no-plusplus
  1720:39   warning  "searchString" is defined but never used                            no-unused-vars
            var separatorCopy = new RegExp(separator.source, flags + 'g');
            string += ''; // Type-convert
            if (!compliantExecNpcg) {

@@ -1007,6 +1007,44 @@ defineProperties($Object, {
// ====
//

var hasNegtiveMonthYearBug = new Date(-3509827334573292).getUTCMonth() === 12;
Copy link
Member

Choose a reason for hiding this comment

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

s/Negtive/Negative

@ljharb
Copy link
Member

ljharb commented Nov 29, 2015

On Opera 11.6, this passes the getUTCMonth test but fails toUTCString and toDateString. On Opera 12.16, this adds a new toDateString failure. On Opera 10.6, this appears to crash my testing browser :-/

@Xotic750
Copy link
Contributor Author

I'll take a further look at this as soon as I get a bit of spare time. I don't have access to Opera 10.6 but I do the other two.

My access to any Opera environments is very limited, at times non-existent. I am still trying to get back to looking at this.

I was able to test Opera 11.6 and it passed, all green, but that's all I have access to at the moment.

I managed to test Opera 12 and that fails the toDateString test, but no fault of the code, the problem is native it has a ',' in the string. So this could be tested and patched, maybe just a replace if you don't want to replace the method?

Been trying to get on browserstack, they have enable me a year free account but my browsers crash when I try to use it, so can't test Opera 10.6

Managed to get on Opera 10.6 and it crashed when I ran tests. :/
Don't know to make of that, I forced the shims and ran tests in all version of node and browsers that I have on my machine and they all ran fine.

This patch now fixes toDateString, toUTCString and toString but it doesn't tackle toLocal methods. IE7-10 now give correct strings but need to test more browsers for any issues though. Opera 10.6 still crashes and I have no idea why?
IE11 and 20 are not taking the toUTCString patch for some reason, they continue to use the native.
Opera 11-12 are working fine.
Firefox 4, 18, 24, 30, 37 & 42 work.
Chrome 26 works.
Safari 5-9 working.

No matter what I've tried, I am unable to get IE11 and 20 to take the toUTCString fix, they continue to use the native method, any ideas?

Ok, I think I've figured out what is going on with IE11 and 20 now, it concerns their representation of negative dates.

That now fixes everything that I can test, except as mentioned the toLocal methods, still don't know what to make of the Opera 10.6 crash or what to do about it?

@Xotic750
Copy link
Contributor Author

Any ideas about discovering the problem with Opera 10.6?

@ljharb
Copy link
Member

ljharb commented Dec 19, 2015

None, sadly. All I can think of is commenting parts out until it doesn't crash, and narrowing down which line does it.

@Xotic750
Copy link
Contributor Author

I've downloaded and installed Opera 10.6 on my systems and started doing just that. I commented out all the tests, it crashed. I commented out all the code, it ran. I started putting code back in and this line (may not be the only one) caused it to crash when reinstated.

var hasToDateStringFormatBug = negativeTestDate.toDateString() !== 'Thu Jan 01 -109252';

I changed the negative date to

var negativeTestDate = new Date(-1509842286000292);

and uncommented the line and it now runs, will try reinstating the rest of the code. But it looks like it doesn't like a date that far in the past with toDateString.

Haven't tried catch or anything yet, but toUTCString also crashes with the original test dates, but not the new date that I posted, and in my terminal it just says

[graham@tyr bin]$ ./opera
Bus error (core dumped)

But I can't find a core dump file.

Nope, try...catch does nothing, it just bums out saying core dump.

I'll see if I can use the original test dates for everything except the ones using toDateString and toUTCString, but I guess there is little that can be done about the core dump

Opera 10.6 now runs without crashing. I added some canned data for months, and I feel it could really use some more for days of the month: just to be 100% sure that all is working, but I don't have time just now. I haven't performed any browser tests on this rebase, other than Opera 10.6
Still, feels like a big step forward.

@ljharb
Copy link
Member

ljharb commented Dec 19, 2015

What was the change that prevented/caused the crash?

@Xotic750 Xotic750 changed the title Add fix for Opera getMonth, getFullYear, getUTCMonth, getUTCFullYear for negative years. Fixes for non-compliant Date, multiple browsers. Dec 19, 2015
@Xotic750
Copy link
Contributor Author

I changed the test date for these detections

// original test date
var hasNegativeMonthYearBug = new Date(-3509827329600292).getUTCMonth() !== 0;
// these two changed because they crashed with original test date
var hasToDateStringFormatBug = new Date(-1509842289600292).toDateString() !== 'Mon Jan 01 -45875';
var hasToUTCStringFormatBug = new Date(-1509842289600292).toUTCString() !== 'Mon, 01 Jan -45875 11:59:59 GMT';

And once detected and patched then they no longer cause a problem with crashing in the tests either. Just the natives broken.

@Xotic750
Copy link
Contributor Author

Added tests for first and last day of each month, Opera 10.6 passing, not tested other browsers.

I was just running a test or two, Opera 11.64 on Window 7 fails 3 tests
https://saucelabs.com/beta/tests/1236492e6c284a9baa456898a03dcd6d/watch
same on Windows XP
https://saucelabs.com/beta/tests/a99c6fb7ee754039b52278cc6e87ee7c/watch
So I downloaded it for my system (Fedora linux) to investigate, and lo and behold it passes all the tests.
https://youtu.be/zyT12Fc2eSA
What to make of that?

Added a fix for the weirdness and now passes Opera 11.64 on Windows and Linux. Tested a few other browsers, IE8,9,10,11 & 20
Opera 10.6 on linux
Opera 11.64 on windows and Linux
Opera 12 on Windows
Safari 5,6,7,8,9
FireFox 4 and 42
Chrome 26 and 47
It's looking pretty good to go, you may want to fill in some other browsers or versions.

One thing that could be improved is the bug detection/tests when the timezone is +1200 to +1400, that's all those little islands along the International Date Line
https://upload.wikimedia.org/wikipedia/commons/8/8a/Timezones2008_UTC%2B10.png
There, the shim will think there are bugs and so things maybe incorrect, as it is a different day/month or year.

That should fix it for the timezone differences with bug detection, tested on my local browsers only, where I have control over the timezone offset. Still need to check the tests.

Opera 11.64 on windows now appears broken again getUTCMonth (1 fail), while working fine on Linux. :/

I seems that the Saucelabs Opera 11.64 is either not emptying its cache or there is a proxy that is serving cached files. This appears to be the reason for the difference, as when I look though the source it is not getting my recent changes.

Doh! I'm an idiot and was pasting the wrong URL!

@Xotic750
Copy link
Contributor Author

I believe that this patch is ready to go, unless you can find any issues with it?
http://browsershots.org/https://rawgit.com/Xotic750/es5-shim/date/tests/index.html

@ljharb ljharb merged commit 6ec68d1 into es-shims:master Dec 27, 2015
@ljharb
Copy link
Member

ljharb commented Dec 27, 2015

All set! There's still some 10.6 issues, but not new ones. I've left those in #240.

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

Successfully merging this pull request may close these issues.

2 participants