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

(v3) Date adapter options are not updated properly. #7342

Closed
iddings opened this issue May 11, 2020 · 2 comments · Fixed by #7346
Closed

(v3) Date adapter options are not updated properly. #7342

iddings opened this issue May 11, 2020 · 2 comments · Fixed by #7346
Milestone

Comments

@iddings
Copy link

iddings commented May 11, 2020

Expected Behavior

this.options in the context of a DateAdapter instance should always reflect the most recent value of chart.options.scales.x.adapters.date (or whichever scale is applicable).

Current Behavior

The initial options are passed to the DateAdapter constructor, and they can be updated once (and only once). Subsequent updates are not propagated. I believe this to be because after the initial render, chart.options.scales.x.adapters.date still references the same object passed to the DateAdapter constructor. After the first update, chart.options.scales.x.adapters.date is replaced with a new object (as a result of config merging).

Possible Solution

I'm not sure if this is the perfect location for this fix, but I was able to fix it by adding:

// update adapter options
me._adapter.options = options.adapters.date;

to TimeScale.buildTicks at scale.time.js line 671

See this CodePen with the change above implemented.

Happy to submit a PR if this is a valid fix.

Steps to Reproduce (for bugs)

  1. Pass options including adapters.date for a time scale.
  2. Change those options, and update the graph (changes are reflected properly).
  3. Change those options again, and update the graph (changes are not reflected after the first update).

In the example, notice that you can switch the timezone from UTC to LST, but you cannot change it back: CodePen

Context

In working to find a solution to #7340, I came across this issue.

Environment

  • Chart.js version: 3.0.0-alpha
  • Browser name and version: 81.0.4044.129
  • Link to your project: N/A
@benmccann benmccann added this to the Version 3.0 milestone May 12, 2020
@benmccann
Copy link
Contributor

benmccann commented May 12, 2020

Yeah, the adapter gets created in the constructor, which only happens once:

const adapter = this._adapter = new adapters._date(options.adapters.date);

The tricky thing here is the order of operations in core.controller:

  1. build the scales
  2. build the elements, which does parsing using the scales
  3. layout, which calls scale.update

A couple options:

  • Can we build the elements after updating the layout? Then we could just override update to update the adapter
  • We could add a new scale.init method which happens in buildOrUpdateScales
    • It should probably take the options as input and so that we can stop doing scale.options = scaleOptions in the controller
  • Just always throw away the scales and rebuild them when updating. I think it'd be cleaner in some ways and reduce the amount of overall code. But maybe there are plugins that rely on doing some expensive one time setup? It'd be a larger migration effort if we change the way it works and it's probably not the easiest to implement

Thoughts @etimberg @kurkle ?

@benmccann
Copy link
Contributor

I sent a PR going the init route since that cleans up some other stuff as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants