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

How to use updateConfig #3886

Closed
dibsyjr1 opened this issue Feb 9, 2017 · 14 comments
Closed

How to use updateConfig #3886

dibsyjr1 opened this issue Feb 9, 2017 · 14 comments

Comments

@dibsyjr1
Copy link
Contributor

dibsyjr1 commented Feb 9, 2017

Hi,

Really liking v2.5.0 (especially being able to update options now). The only problem i'm having is how to call updateConfig. I've tried a few different things including:

(for the sake of these examples, the chart instance will be called chartInst)

  • Chart.updateConfig(chartInst)
  • chartInst.updateConfig(newOptions)
  • chartInst.updateConfig()
  • updateConfig(chartInst)

But none of these work and there's nothing in the documentation to explain how to use this new feature. Is anyone able to give me some insight into how it's used?

@etimberg
Copy link
Member

etimberg commented Feb 9, 2017

The final api ended up folding config updates into regular updates.

So you can apply changes to chartInst.options and then calling chartInst.update

@dibsyjr1
Copy link
Contributor Author

dibsyjr1 commented Feb 9, 2017

Ahhhh, okay, brilliant, thanks @etimberg!

@etimberg etimberg added this to the Version 2.6 milestone Feb 12, 2017
@AgamlaRage
Copy link

I couldn't acheive this using .options
Here is the solution that works for me #593 (comment)

myChart.config.data = d; // d is my new dataset
myChart.update();

@vdh
Copy link

vdh commented Feb 23, 2017

@etimberg

This really simple attempt at updating options fails horribly:

chart.options = newOptions;
chart.update();

This would be much less of a headache if the options object (and data, while we're at it) wasn't mutated internally…

@simonbrunel
Copy link
Member

@vdh data is now fully supported starting v2.6 (chart.data = newData), so we can do the same for options (would be a bit different since we need to handle defaults).

@etimberg
Copy link
Member

One complication with changing the entire options object is that you may end up needing to effectively destroy and recreate the entire graph. Obviously new defaults have to be merged, but axes could change and that leads to a lot of complications

@vdh
Copy link

vdh commented Feb 23, 2017

@etimberg This isn't a very approachable design when trying to do functional programming. I shouldn't have to worry about hidden minefields like that when trying to update a few options.

Take for example, the scale options, which auto-generate ids if none are defined. There was no indication anywhere that this would break when trying to update options, because suddenly that id is gone when that section of the options object is replaced.

A better approach would have been to store that important id inside something internal, or even better, require those ids to begin with so you would have a unique identifier when writing code to update those options. Ideally, both store it internally and require ids (at least for people expecting to use .update).

Not everyone is going to have a picturesque scenario where they know exactly where they need to drill down into a data structure to make their updates. I'm using React and Immutable.js, so I basically had to cobble together something vaguely safe using lodash's mergeWith. But I can't be sure if my hack will actually be safe because that options object is an undocumented bundle of mystery.

@etimberg
Copy link
Member

@vdh I agree that this is not great behaviour, especially from a functional perspective.

I think we can definitely improve things on the axis side. Some options I see are:

  1. Instead of replacing the old options with the new one, extend the old one with the new one. That way properties like the ID would not change.
  2. Manually copy over the ID & other properties during the replacement.

Both options would modify: https://github.com/chartjs/Chart.js/blob/master/src/core/core.controller.js#L47-L53

I think a potential (untested) work-around would be to do the following. In essence, it merges the new config on top of the old config and sets that. That way mutated or merged properties are not lost

// assumes newConfig is already known here
newConfig = Chart.helpers.extend(chart.config, newConfig);
chart.config = newConfig;
chart.update();

@vdh
Copy link

vdh commented Feb 26, 2017

@etimberg That's exactly what I've had to do to update the options:

import mergeWith from 'lodash/mergeWith';

const chartJsCustomizer = (existingValue, newValue) => {
  if (Array.isArray(existingValue)) {
    if (
      Array.isArray(newValue) &&
      existingValue.length === newValue.length &&
      existingValue.length > 0 &&
      typeof existingValue[0] === 'object'
    ) {
      // Two arrays of same length with objects, mutate each item instead
      existingValue.forEach((item, index) => {
        mergeWith(item, newValue[index], chartJsCustomizer);
      });
      return existingValue;
    }
    // Replace old array with new value
    return newValue;
  }
  // Default to lodash merge logic
  return undefined;
};

// later…

// Update the options
mergeWith(chart.options, newOptions, chartJsCustomizer);

But the caveats are:

  • It only works if nothing is added/removed to arrays.
  • Can't remove options that aren't explicitly null-ed, due to the merge strategy.
  • It will silently fail if an array length doesn't change due to an even number of additions & removals, because there's no proper way to track the identity of array items without hardcoding checks for specific options.

This is the kind of rabbit hole that happens when objects are mutated instead of copied 😢

@etimberg
Copy link
Member

One of the things we did think about is trying to get rid of the array xAxes and yAxes properties. This would at least make the merging cleaner since one option was to use the ID of the axis as the key of some nested config. This would end up making things more straightforward, but I'm not yet convinced we can maintain backwards compatibility.

As much as I would love to remove the config merge with the global config, it does keep the library smaller because we don't have to check for defaults in a lot of different places. Maybe we need to keep the original config around somewhere so that we know what the unmerged state looked like.

@ryan-morris
Copy link

@etimberg I still seem to be having trouble with updating the options. It seems to be specifically be coming down to scale id issues. codepen example , maybe it's something I'm doing

@etimberg etimberg modified the milestones: Version 2.7, Version 2.6 May 3, 2017
@benmccann
Copy link
Contributor

Let's close this as a duplicate of #4197. That one has a pending PR. Both issues should be fixed when the PR is committed

@etimberg
Copy link
Member

etimberg commented Jun 8, 2020

I tested this in v3.0.0-alpha. We've replaced the xAxes and yAxes arrays with a scale configuration that each axis' config mapped by ID.

I built https://jsfiddle.net/zse3qmfb/1/ from the 'toggle scale type' sample but updated it to replace the options object on change. It seems to work fine.

@etimberg
Copy link
Member

etimberg commented Oct 4, 2020

Closing per my previous comment. We're up to v3.0.0-beta.3 now and this is still working. Updated fiddle: https://jsfiddle.net/30c45yLd/

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

No branches or pull requests

7 participants