chartjs / chartjs-adapter-luxon

Luxon adapter for Chart.js
MIT License
33 stars 24 forks source link

Passing Zone object to options.adapters.date causes invalid dates. #18

Closed iddings closed 3 years ago

iddings commented 4 years ago

This isn't really a bug with this adapter, but it doesn't seem like this warrants an issue in the Chart.js main repo.

Because of the way Chart.js merges options, things like a luxon FixedOffsetZone get broken. I.e. if you pass a FixedOffsetZone object as the "zone" option to this adapter, Chart.js will convert it to an object like this: { fixed: -300 }. Luxon will then see that as an invalid zone.

One fix for this could be catching that edge case converting the zone from (e.g.) { fixed: -300 } to simply -300, which Luxon will handle properly.

Locally I've worked this out by adding:

/**
 * @private
 */
_normalizedOptions: function () {
    if (
        'object' === typeof this.options &&
        'zone' in this.options &&
        'object' === typeof this.options.zone &&
        'fixed' in this.options.zone &&
        'number' === typeof this.options.zone.fixed
    ) {
        const merged = Chart.helpers.merge({}, this.options);
        merged.zone = this.options.zone.fixed;
        return merged;
    }
    return this.options;
},

and replacing references to this.options with this._normalizedOptions() elsewhere.

If this is deemed not applicable to this repo, then I would suggest updating the documentation to indicate that only certain types of Zones are allowed.

I can make sure all types of Zones are supported and submit a PR if that's preferred.

Update: changed FixedOffset => FixedOffsetZone

benmccann commented 4 years ago

Oh, so the issue is that Chart.js expects all options to be object literals and if you pass in an instance of some other type then Chart.js clones it and you lose the class instance. Is that it?

If I've correctly described the issue, I think the best thing to do would be to report it in the main Chart.js repo and see if we can get a real fix for the underlying issue. We're currently working on Chart.js 3, so if we need to change the behavior of options in a breaking way to fix it this would be the time to do it

benmccann commented 4 years ago

Linking the upstream issue: https://github.com/chartjs/Chart.js/issues/7340

benmccann commented 4 years ago

@iddings thanks for filing the issue upstream

I just took a look at the Luxon docs and didn't see any examples of using a FixedOffset. All the examples used strings to specify time zones. Can you share an example of how to reproduce this and what input you're providing?

iddings commented 4 years ago

Sure, FixedOffset is a typo, should have been FixedOffsetZone. Our use case is displaying climate data in either UTC, or local standard time (of the station at which the data was collected). We essentially pretend that DST doesn't exist, so we use fixed offsets from UTC to present data.

CodePen

Without a workaround, passing an instance of FixedOffsetZone results in no data being displayed (because Luxon sees the object it is passed as an invalid zone).

This example has a relatively easy workaround (i.e. using a number instead of a FixedOffsetZone instance), however, in our actual use case, we're accepting any valid Luxon zone (string, number, or any instance that implements luxon.Zone) from an external source, and passing it to the adapter options in Chart.js. This makes it impossible to convert every possible zone to something that Chart.js can handle, as the external source could pass in an instance of a custom class which implements luxon.Zone.

I'll add this example to the upstream issue as well.

etimberg commented 3 years ago

This should be resolved in chart.js v3.0.0-beta.12 since options are no longer merged