Bug: Using a date (vs a datetime) results in timezone shifts

[daattali]

Copied over from almende/vis#3851 and yotamberk/timeline-plus#100

In old versions (4.16), using a date string of "2013-04-20" would result in this:

Since then, using the same string results in:

Notice that even though I specified April 20, the point on the timeline is on the 19th.

My hypothesis is that when no time+timezone is provided, visjs automatically assumes midnight UTC and then converts it to the user's local time. The expected behaviour would be to have the pin exactly on Apr 20 midnight. Having the pin on a day earlier looks broken.

[strazto]

I'm looking into this because I'm interested in resolving some of the outstanding regressions (Or at least gripes) in vis-timeline that are preventing @daattali from updating the embedded timevis version in his popular (>= 424 stars) R package https://github.com/daattali/timevis

I'd like to rephrase your description of the current vs expected behaviour to make sure I understand it:

Current

A date with no time/zone qualification

Providing a date with no time+timezone qualification (eg "2020/04/20" ) appears to be interpreted as midnight UTC: `"2020/04/20T00:00:00+00:00".

A date with a time qualification but no zone

Conversely, providing a date + time without a timezone qualifier appears to be intepreted in local time, so in my case (UTC+10:00):

"2020/04/20 00:00:00" -> `"2020/04/20T00:00:00+10:00"

According to wikipedia on ISO8601 this intepretation of an unqualified datetime string as being in local time is ISO compliant.

The problem

This behaviour is likely to confound users, particularly as it changed between minor versions without an explicit announcement, as a user's timeline is likely rendered in local time, & if they supply the following 2 entries:

• "2020-04-20"
• "2020-04-20 00:30:00"

They would reasonably expect these two values to appear to be 30 minutes apart.

Digression into the ambiguity of ISO8601 wrt this question

Furthermore, the ISO8601 standard does not specify how dates should be handled with respect to timezones.

Dates are dates under ISO8601, & their specificity ends there, which makes it hard to reason whether a given (fully qualified) ISO8601 datetime falls under a ISO8601 date -

For example:

For the following 2 timestamps:

date_stamp = "2020-04-20"

# Local time: 2020-04-20 2:01PM 
# UTC time:  2020-04-21 12.01AM UTC
time_stamp = "2020-04-20T14:01:00+10:00"

# The following is ambiguous
time_stamp ∈ date_stamp 

"Best" behaviour

There's not really a "right" answer as to how to handle this.
Assuming UTC often feels like a safe bet,

BUT if an unqualified timestamp is assumed to be local, (And this is the ISO compliant behaviour) I would reason that the least astonishing behaviour is to treat an unqualified date similarly.

This being said - Perhaps we can have it all ways by adding an option to instances of the timeline date_offset_UTC, with a sensible default (either a: UTC, or b: whatever the timezone of the this instance is- I'm leaning toward b).

I'm open to discussion on how to name this option, as date_offset_UTC feels clumsy, but if it's okay, I'd like to address this

[daattali]

Thanks for the initiative!

Your description of the problem is accurate.

I would strongly argue that regardless of this being a breaking change, the current behaviour is broken, even if it would have always been this way. Assuming UTC does indeed feel like a safe bet in most contexts. But in this case, if someone says "place a pin on April 5" and you see a pin on April 4 at a random hour, it just looks broken. This is a case where assuming UTC is not the correct behaviour.

I think adding an option would be a great solution. Either "default_timezone" or "timezone_offset" are good options to me.

[strazto]

I've found the offending line:

https://github.com/visjs/vis-timeline/blob/f95eb73/lib/util.js#L61-L84

Specifically:

https://github.com/visjs/vis-timeline/blob/f95eb73/lib/util.js#L76

Discussion of moment.js vs builtin Date parsing

What are we seeing here?

• When attempting a string to "Date":
  • Not applicable to us
    If the string matches the "ASPDate" format ( I don't know or care about this format spec ), we use that to construct the date
• If the string isn't of "ASPDate" format, construct a Date from it using new Date(), a construct a moment from the resulting object.

Taking a dive into the behaviour of moment.js:

According to the docs on parsing:

• moment(...) is local mode. Ambiguous input (without offset) is assumed to be local time. Unambiguous input (with offset) is adjusted to local time.
• moment.utc(...) is utc mode. Ambiguous input is assumed to be UTC. Unambiguous input is adjusted to UTC.
• moment.parseZone() keep the input zone passed in. If the input is ambiguous, it is the same as local mode.
• moment.tz(...) with the moment-timezone plugin can parse input in a specific time zone.

moment's default behaviour when input is ambiguous is to treat it like local time.
In our case, moment is not doing the parsing, but rather js's builtin Date is responsible.

If we're wondering what the behaviour of the parser in the new Date() constructor is, it turns out that parsing of date strings using js Date() is strongly discouraged:

Note: Parsing of date strings with the Date constructor (and Date.parse, they are equivalent) is strongly discouraged due to browser differences and inconsistencies.

Addressing this

Consider the following:

<!DOCTYPE HTML>
<html>
<head>
  <title>Timeline | Basic demo</title>

  <style type="text/css">
    body, html {
      font-family: sans-serif;
    }
  </style>

  <script src="../../standalone/umd/vis-timeline-graph2d.min.js"></script>
  <link href="../../styles/vis-timeline-graph2d.min.css" rel="stylesheet" type="text/css" />

</head>
<body>

<p>
  A basic timeline. You can move and zoom the timeline, and select items.
</p>

<div id="visualization"></div>

<script type="text/javascript">
  // DOM element where the Timeline will be attached
  var container = document.getElementById('visualization');

  // Create a DataSet (allows two way data-binding)
  var items = new vis.DataSet([
    {id: 1, content: "start: '2020-04-20', end: '2020-04-21'", start: '2020-04-20', end: '2020-04-21'},
    {id: 2, content: 'dttm 2pm + offset <br> 2020-04-20T14:01:00+10:00', start: '2020-04-20T14:01:00+10:00', type:  "point"},
    {id: 3, content: 'midnight - offset <br> 2020-04-20T00:00:00', start: '2020-04-20T00:00:00', type: "point"},
    {id: 4, content: 'midnight + offset <br> 2020-04-20T00:00:00+00:00', start: '2020-04-20T00:00:00+00:00', type: "point"}
  ]);

  // Configuration for the Timeline
  var options = {
    // Display in AEST, +10:00, so others can reproduce
    moment: function(date) {
      return vis.moment(date).utcOffset('+10:00');
    }
  };

  // Create a Timeline
  var timeline = new vis.Timeline(container, items, options);
</script>
</body>
</html>

at f95eb73 this renders out to:

Changing

https://github.com/visjs/vis-timeline/blob/f95eb73/lib/util.js#L76

to:

          return moment(new Date(object)).toDate(); // parse string

This renders out to:

We see that our problem is immediately fixed.

The drawback

Unfortunately, when running npm run test with this fix applied,

https://github.com/visjs/vis-timeline/blob/f95eb73/test/util-convert.test.js#L48-L50

Throws the following warning:

Deprecation warning

Deprecation warning: value provided is not in a recognized RFC2822 or ISO format. moment construction falls back to js Date(), which is not reliable across all browsers and versions. Non RFC2822/ISO date formats are discouraged and will be removed in an upcoming major release. Please refer to http://momentjs.com/guides/#/warnings/js-date/ for more info.
Arguments:
[0] _isAMomentObject: true, _isUTC: false, _useUTC: false, _l: undefined, _i: 1198908717056, _f: undefined, _strict: undefined, _locale: [object Object]
Error:
    at Function.createFromInputFallback (/home/matty/work/repos/vis-timeline/node_modules/moment/moment.js:319:25)
  ... (massive stack trace)

Essentially, if the input string isn't ISO/RFC compliant, moment.js falls back to the built-in new Date() parser, and this parser sucks (/is inconsistent, which sucks).

Basically, moment.js falls back to doing what we were doing anyway, and what we were doing was a bad idea so it complains.

I'm not sure why we originally constructed our moments via new Date()s in the first place, anyway - Possibly just to suppress this warning?

Checking the commit log confirms this:

3611162#diff-3deb3f32958bb937ae05c6f3e4abbdf5

@daattali this is the commit that introduced our regression:

vis-timeline/package.json

Lines 1 to 4 in 3611162

	{
	"name": "vis",
	"version": "4.16.1",
	"description": "A dynamic, browser-based visualization library.",

This is consistent with @daattali 's findings that this regression occured > version 4.16

Conclusion

Squashing a warning is a bad reason to break stuff. I concur with Dean that this is %100 a bug, and the best behaviour is to let moment parse the string.

To accomodate for this warning squash, for example, our test case:

vis-timeline/test/util-convert.test.js

Lines 44 to 50 in f95eb73

	it("converts to Date from Number", function() {
	assert(util.convert(1198908717056, "Date") instanceof Date);
	});
	it("converts to Date from String", function() {
	assert(util.convert("1198908717056", "Date") instanceof Date);
	});

Which essentially aims to check that we're able to pass an epoch timestamp, both numerically, or as a string to the conversion, I will allow moment to explicitly check RFC or ISO, & defer more gracefully to js Date if the format doesn't match as per:

https://stackoverflow.com/a/30870755/9238801 which suggests the following to validate an ISO compliant datestring:

moment("2015-06-22T13:17:21+0000", moment.ISO_8601, true).isValid(); // true

Sadly, from docs it doesn't appear that moment also exports a moment.RFC_3339, so we'll probably have to roll our own format spec for RFC_3339.

I'll probably update the test-cases to include tests for our particular problem:

util.convert("2020-04-20", "Date") === util.convert("2020-04-20T00:00")

as well as validating that

util.convert("1198908717056", "Date") === util.convert(1198908717056, "Date")

[vis-bot]

🎉 This issue has been resolved in version 7.3.10 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀