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

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

Closed
iddings opened this issue May 5, 2020 · 5 comments
Closed

Comments

@iddings
Copy link
Contributor

iddings commented May 5, 2020

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
Copy link
Collaborator

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
Copy link
Collaborator

Linking the upstream issue: chartjs/Chart.js#7340

@benmccann
Copy link
Collaborator

@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
Copy link
Contributor Author

iddings commented May 11, 2020

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
Copy link
Member

etimberg commented Mar 7, 2021

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

@etimberg etimberg closed this as completed Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants