Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Test for native iso8601 date support #167

Closed
wants to merge 2 commits into from

2 participants

@vipero07

Added a test for native iso8601 to date support. This should allow for browsers that support ECMAScript 5 to skip the regex replaces.

@vipero07 vipero07 Test for native iso8601 date support
Added a test for native iso8601 to date support. This should allow for browsers that support ECMAScript 5 to skip the regex replaces.
1f5a7af
@rmm5t

Please read Pull Request Etiquette, especially "3. Stick to Existing Conventions."

Also, if a flag variable like this is used, I'd prefer to see it set in the positive, not the negative. More on this below.

  var $t = $.timeago;
  var iso8601Support = Date.prototype.toISOString;
@rmm5t

Please always stick to the same syntax conventions of the original project, including use of whitespace.

if (!iso8601Support) {
  // ...
}
@rmm5t
Owner

One more thing. This PR currently breaks the test suite. Chrome (at least) can't parse a string like "1978-12-19T02:17:00+09" (probably because of the short time zone value). We might need to keep the last regex replacement in all scenarios.

This one:

        s = s.replace(/([\+\-]\d\d)$/," $100"); // +09 -> +0900

The ISO8601 spec says the following are valid time zone designators:

  • <time>Z
  • <time>±hh:mm
  • <time>±hhmm
  • <time>±hh
@vipero07 vipero07 Modified code to better match existing conventions
Changed the iso8601support variable to be set in the positive, also moved the final replace out of the if check because its absence breaks the test suite on chrome. See rmm5t#167 for more information.
c4f4315
@vipero07

I made the above changes, I find it odd that chrome doesn't properly support valid time zone designations, but I moved the last regex out of the if statement.

@rmm5t
Owner

@vipero07 It doesn't look like you're running the test suite before pushing changes. It's still broken in Chrome.

I was able to modify these changes slightly to get things working in Chrome...

      if (!iso8601Support) {
        // ...
        s = s.replace(/([\+\-]\d\d)$/, " $100"); // +09 -> +0900
      } else if (s.length > 10) { // long enough to include time
        // Chrome doesn't understand short time zone designators
        s = s.replace(/([\+\-]\d\d)$/, "$100"); // +09 -> +0900
      }

...HOWEVER Safari and Firefox are both severely broken. It doesn't look like the major browsers agree how to parse iso8601 timestamps, so I'm going to have to close this pull-request. It's not worth the effort.

For future reference, please only submit pull-requests after you've guaranteed that the test-suite is passing on major browsers.

@rmm5t rmm5t closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 15, 2014
  1. @vipero07

    Test for native iso8601 date support

    vipero07 authored
    Added a test for native iso8601 to date support. This should allow for browsers that support ECMAScript 5 to skip the regex replaces.
Commits on Feb 16, 2014
  1. @vipero07

    Modified code to better match existing conventions

    vipero07 authored
    Changed the iso8601support variable to be set in the positive, also moved the final replace out of the if check because its absence breaks the test suite on chrome. See rmm5t#167 for more information.
This page is out of date. Refresh to see the latest.
Showing with 7 additions and 4 deletions.
  1. +7 −4 jquery.timeago.js
View
11 jquery.timeago.js
@@ -35,6 +35,7 @@
}
};
var $t = $.timeago;
+ var iso8601Support = Date.prototype.toISOString;
$.extend($.timeago, {
settings: {
@@ -103,10 +104,12 @@
},
parse: function(iso8601) {
var s = $.trim(iso8601);
- s = s.replace(/\.\d+/,""); // remove milliseconds
- s = s.replace(/-/,"/").replace(/-/,"/");
- s = s.replace(/T/," ").replace(/Z/," UTC");
- s = s.replace(/([\+\-]\d\d)\:?(\d\d)/," $1$2"); // -04:00 -> -0400
+ if (!iso8601Support) {
+ s = s.replace(/\.\d+/,""); // remove milliseconds
+ s = s.replace(/-/,"/").replace(/-/,"/");
+ s = s.replace(/T/," ").replace(/Z/," UTC");
+ s = s.replace(/([\+\-]\d\d)\:?(\d\d)/," $1$2"); // -04:00 -> -0400
+ }
s = s.replace(/([\+\-]\d\d)$/," $100"); // +09 -> +0900
return new Date(s);
},
Something went wrong with that request. Please try again.