Support timezone config

[jonrimmer]

We need to display our charts in UTC, so will have to fork this adapter to do this. Would be useful if there was some way to supply a timezone param to be used in the create function. E.g.

function create(time) {
  return DateTime.fromMillis(time, { zone: config.zone });
}

[simonbrunel]

That's something planned since we introduced adapter options. But until it's implemented (and if all your charts need to be UTC), you can already configure the default zone globally:

import { Settings } from 'luxon';

Settings.defaultZoneName = 'utc';

[jonrimmer]

Thanks Simon. 2.8 is working great for us so far!

[simonbrunel]

and if all your charts need to be UTC

I think that's the most common scenario (i.e. all charts with the same time zone) and in this case, I wouldn't even use adapter options but directly manipulate the Luxon default settings API. When different time zones are needed for charts on the same page, then yes, we will need adapter options, something like:

new Chart(ctx, {
  options: {
    scales: {
      xAxes: [{
          type: 'time',
          adapters: {
            date: {
              zone: 'utc'
            }
          }
      }]

    //...
});

[jonrimmer]

Yeah. The only drawback is that it's a big app with various feature modules, of which the chart/reporting part is only one. So somebody else might want to show local timezone dates in a different part of the app, and not realise the default is being overwritten to UTC.

EDIT: So, in case it helps anybody else, my solution to this (I'm using Angular) is to set and reset the default timezone when my component loads and unloads. E.g.

import { Settings } from 'luxon';

class ChartingComponent implements OnInit, OnDestroy {
  private originalDefaultTimezone: string;

  ngOnInit() {
    this.originalDefaultTimezone = Settings.defaultTimezoneName;
    Settings.defaultZoneName = 'utc';
  }

  ngOnDestroy() {
    Settings.defaultZoneName = this.originalDefaultTimezone;
  }
}

[simonbrunel]

Did you figure out how to make moment optional in Angular projects? If so, maybe you can help @lsn793 about this issue.

[benmccann]

@jonrimmer I wanted the same feature, but probably won't be able to work on it for awhile since there are some other improvements I'm working on in the meantime. If you wanted to take a stab at it, I'd help review and test it though

With the adapter options PR, you should be able to set an option at options.adapters.date and have it passed into the adapter where it can be used

[jonrimmer]

@benmccann I'm happy to take a look at this. How do you think we should approach it? I see a couple of options:

1. Create as thin a bridge as possible, with a createOpts adapter option that gets passed as the Luxon options argument into every call to fromMillis, fromFormat, fromISO, etc. The advantage of this is that it lets users leverage all the possible options Luxon supports without Chart.js having to know anything about them E.g. not just zone, but also locale, outputCalendar and others, including any added in future. The potential downside is that the adapter's API and behaviour becomes a bit nebulous, as it's now defined by what the version of Luxon the user is using supports, and there might be options that cause problems for Chart.js.

2. Create a more narrow bridge, where we only look for particular defined options, like zone and explicitly map them to the equivalent Luxon option. The advantage is that this retains tighter control over the adapter's behaviour and API. The disadvantage is that every option and use-case must be explicitly handled by the adapter.

I think my preference would be for the first option.

[simonbrunel]

@jonrimmer options are already available as part of this.options from all adapter methods. These options are (currently) read from the time scale adapters.date options (as my previous example). Chart.js doesn't know about the content of this object which is directly passed to the adapter.

So basically, we just need to inject these options in Luxon calls when applicable. I agree that we also need to decide if (1) we should pass this.options directly to Luxon (e.g. DateTime.fromMillis(time, this.options) or (2) if we should sanitize / map them to keep control about what options the adapter exposes and how.

(1) would be easier to integrate and more optimized, but specific to Luxon (maybe not an issue) (2) would maybe help to standardize the options API between all date adapters

Let's bring @kurkle in this discussion, he may also have great inputs.

[kurkle]

I would start by passing this.options directly to Luxon. Sanitizing / mapping from standard options API (TBD) can be added later, if it seems like a good idea.

If we later go for standardized options case I would still pass non-standard options directly, so we don't limit the possibilities and don't need to update adapters when underlying lib evolves.

Luxon's locale and zone sound like a good candidates to be standardized, if going that path.

[benmccann]

Here's a list of options that a Luxon DateTime can take: https://github.com/moment/luxon/blob/master/src/datetime.js#L519

Info for outputCalendar here: https://moment.github.io/luxon/docs/manual/calendars.html And numberingSystem here: https://moment.github.io/luxon/docs/manual/intl.html#numberingsystem

zone and locale will be the most useful. outputCalendar would be possibly useful.numberingSystem doesn't sound very useful at all. But there's only a few and it probably doesn't hurt to just pass whatever the user specifies directly. That would have the benefit of automatically supporting any new options that Luxon adds

I'd probably nest these options under a dateTime key so that we have the possibility of providing additional types of options in the future

[simonbrunel]

adapters.date.dateTime.zone is quite redundant and confusing. We could nest under something more meaningful (e.g. adapters.date.luxon.zone) but I don't think it worths the extra level since most options will target Luxon. I also don't think users should care about which options are passed to Luxon directly or not.

[kurkle]

In general, we have too much nesting from user point of view. So I would not nest. I don't think there will be any overlap, unless adapter is change to do something its not designed to be doing.

[benmccann]

If we don't want to separate options by dateTime, duration, etc. then let's include a test that verifies that Luxon will accept unexpected options so that if a user sets both DateTime and Duration options that the DateTime won't error from receiving Duration options and vice versa

[simonbrunel]

I didn't understand why the dateTime key but now I get it was to scope by Luxon types (DateTime, Duration, Info and Interval). I'm not sure we need to do that because even Luxon doesn't make a difference, for example Interval.fromISO accepts a single opts object, which one will be "pass to DateTime.fromISO and optionally Duration.fromISO" (code).

[benmccann]

They're not always exactly the same. E.g. there's conversionAccuracy as a new option: https://github.com/moment/luxon/blob/master/src/duration.js#L201

I'm still not sure it's necessary to separate them out because it might get annoying to specify the same option for every luxon type. E.g. you probably don't need to set locale differently for Duration, DateTime, Interval, etc. As long as we make sure they accept unexpected options it's probably okay

[jonrimmer]

Any thoughts on how to write tests for this?

• Do we need proper integration tests with Chart.js? I think this would be dependent on the creation of a chartjs-devtools package?
• If unit tests alone are acceptable, should these still use Karma, or something else (e.g. Jest)?

[benmccann]

All the Chart.js tests (including for plugins) use Karma. I just setup tests for https://github.com/chartjs/chartjs-chart-financial. It has a dependency on Chart.js and shows how to setup a test using that dependency. This was mostly copied from https://github.com/chartjs/chartjs-plugin-datalabels, which might have a bit cleaner setup. I don't think chartjs-devtools package would necessarily be a blocker. If we have a little duplication in the short-term that seems fine to me because it would help demonstrate which functionality might be shared between projects.

[jonrimmer]

@benmccann The adapter only uses DateTime, not Interval or any of the other Luxon types, though. So is checking for options compatibility between them still necessary for this project?

[benmccann]

I think it does use some other types. E.g. we call DateTime.diff which returns a Duration. But that being said I think it's probably cleaner not to have the extra level of nesting, so I'm okay not worrying about it