fix scale options update error #4198

xg-wang · 2017-04-29T08:10:49Z

#4197
When updating the options, should merge with the default value and add ids before setting them, just like when initializing.

And I think the function name should updateOptions since it really only touched the options.

etimberg

Definitely needs tests before we could merge this

etimberg · 2017-04-29T12:18:22Z

src/core/core.controller.js

+			newOptions = helpers.configMerge(
+				Chart.defaults.global,
+				Chart.defaults[chart.config.type],
+				newOptions || {});


I think this can just be newOptions since it already had to have the scale or scales properties on line 47

Thanks for the advice, I will change this.
I can see it working fine by building the new bundle. Should I write a test case for this? Can you suggest a test file for me to add one, I'd love to contribute one.

I think the best location for tests would be in https://github.com/chartjs/Chart.js/blob/master/test/specs/core.controller.tests.js 😄

This is actually creating a new options object every update. A flag set by the setter of chart.options.scales can be used to skip unnecessary change.
As the update function isn't called frequently I guess it's ok to update every time?

etimberg

sorry for the delay in reviewing this. I added a question about what happens when axes are added or removed

etimberg · 2017-08-10T00:59:36Z

src/core/core.controller.js

+			if (newOptions.scale) {
+				chart.scale.options = newOptions.scale;
+			} else if (newOptions.scales) {
+				newOptions.scales.xAxes.concat(newOptions.scales.yAxes).forEach(function(scaleOptions) {


What happens in the case where new axes are added? I think we'd need to create those scale objects somewhere in here. Similarly, axes could be deleted.

Sorry just noticed #3886, will take some time to investigate.

xg-wang · 2017-08-10T06:03:55Z

From the behavior perspective, I assume

if the user reset the option object or option.scales.{xAxes,yAxes}, we create a new option object by merging it with default config - which is just the same as initialization did. Here we handle creating/destroying new/old axes.
to avoid creating new option or axes, user should only mutate the field property (in this case all previously merged properties are not reset), then we don't destroy old axes.

What I haven't decided: if the new option being set doesn't have axes specifed, should we assume the old axes being used? 2 situations when creating chart:

config.options.scales has axes: should we assume user don't want the axes anymore or they want the axes to remain the same?
config.options.scales has no axes: I believe we shouldn't bother to destroy/create again the axes we generated.

etimberg · 2017-08-11T01:01:46Z

@xg-wang good questions!

If there are axes specified in the new options, we should replace the old axes with the new axes. This may end up being too complicated to do.

I think if there are no new axes then we should keep the old ones and perhaps throw an error, or put a console warning.

@simonbrunel looping you in for your thoughts

xg-wang · 2017-08-13T18:04:29Z

@etimberg
new commit implemented a property setter to set flags to indicate option update strategy, which would be used in updateConfig, this looks a bit hacky but it does work and I think the performance is not bad.
I tested on some samples and tests, will add more tests for all discussed option update method to it if this looks good.

This is the first time I made quite some changes to Chat.js so please advise if anything's wrong or breaking the style.

xg-wang · 2017-08-13T18:21:30Z

Previously chart.options is an alias to chart.config.options, so when user mutates chart.options.xxx or chart.options.scales.xxx we can't really tell whether should rebuild scales or not. Thus I set the flags for deciding to rebuild scales.

simonbrunel · 2017-08-13T18:38:48Z

The "scales" arrays strikes back :\

I agree that it looks hacky and since we are planning to move away from these arrays, I'm a bit worry of introducing this sync logic. There are so many way to update the options in a way that would break the update (I think chart.options.scales.xAxes.shift/unshift doesn't work).

I'm wondering if it would not be easier and more robust to simply destroy and recreate all scales at every update? what would be the consequences to do that instead of trying to re-sync scales? why would we want to sync scales in the first place (it doesn't support animations)?

xg-wang · 2017-08-13T19:04:59Z

Yes it would be much more easier and cleaner if destroy every time.
The work recreating does is transform scales areay to chart.scales and add to chart.boxes. not much effort actually.

xg-wang · 2017-08-13T23:30:29Z

@simonbrunel
The only consequence I can think of is that user would lose vars that point to scales.
I changed the option update to force rebuild scales every time and some tests would fail.
e.g. https://github.com/chartjs/Chart.js/blob/master/test/specs/scale.linear.tests.js#L751
the var for xScale would be discarded when chart.buildScales().
I inspected other failed tests, seems all failed because of the lost reference.

xg-wang

I have to update these tests to pass CI, but not sure these would affect some users' use cases or not.

etimberg · 2017-08-14T01:47:30Z

@simonbrunel once consequence is a performance penalty for update. Now, we have to re-merge all the axis options and see what changes.

@xg-wang is it possible to profile these changes and see what the performance penalty looks like?

xg-wang · 2017-08-14T02:17:45Z

@etimberg ok I'll profile and compare the two update strategy.
@simonbrunel btw can you show me the issues discussing the scales array?

etimberg · 2017-08-20T12:06:21Z

src/core/core.controller.js

+					scales[scale.id] = scale;
+				}
+				// clear up discarded scales
+				helpers.each(updated, function(id, hasUpdated) {


I'm not sure that this loop is in the right place. I think it needs to happen after the entire outer loop has run. On the first iteration of the outer loop, only 1 scale will be updated and this will delete all the other axes

xg-wang · 2017-08-20T21:13:34Z

src/core/core.controller.js

-		}
-		chart.config.options = chart.options;
+
+		chart.options = chart.config.options = newOptions;


Since a new object has already been created, it should be more clear to just reset the options, the result is actually the same.

xg-wang · 2017-08-20T21:18:33Z

src/core/core.controller.js

@@ -291,6 +297,14 @@ module.exports = function(Chart) {
 					me.scale = scale;
 				}
 			});
+			// clear up discarded scales
+			helpers.each(updated, function(hasUpdated, id) {


@etimberg last commit here was definitely a mistake. Fixed here.
So now whenever user specify a scale with a new id or the type is changed, user lose the reference to the old scale.

xg-wang · 2017-08-20T21:23:45Z

src/core/core.controller.js

@@ -314,6 +328,7 @@ module.exports = function(Chart) {

 				if (meta.controller) {
 					meta.controller.updateIndex(datasetIndex);
+					meta.controller.linkScales();


I came across this when I tried to update scales with different id - dataset._meta contains scales id which didn't update.
I check the scales id of meta here, relink the scale if discarded.

I'm not sure whether I missed anything else, please point out if there are. It's a little bit too many configs here : P

xg-wang · 2017-08-20T21:31:31Z

samples/scales/toggle-type.html

+    document.getElementById('randomizeData').addEventListener('click', function() {
+        type = type === 'linear' ? 'logarithmic' : 'linear';
+        window.myLine.options.title.text = 'Chart.js Line Chart - ' + type;
+        window.myLine.options.scales = {


If the type of the scale was to be changed, the scales object has to be created instead of modify the property. This is for ticks is still using the old type scale's ticks - and is being merged when updating the options.scales object.
I think we should suggest updating scales options by recreating a whole object unless user is confident there is nothing wrong there.

@xg-wang I am ok with that stipulation as long as we add it to the documentation

xg-wang · 2017-08-20T22:11:38Z

Simple performance test on sample file toggle-type.html

rebuild scales every time

new

construct: 20.01
- initConfig: 0.67
- initialize: 4.72
  - buildOrupdateScales: 0.52
- update: 13.2
  - updateConfig: 0.66
    - helpers.configMerge: 0.53
    - buildOrUpdateScales: 0.13
  - buildOrupdateControllers: 0.51
  - updateLayout: 8.34
render: 10.17

update

update: 3.25
- updateConfig: 0.53
  - helpers.configMerge: 0.39
  - buildOrUpdateScales: 0.14
- buildOrupdateControllers: 0.13
- updateLayout: 2.15
render: 6.35

snapshot is for page reload - 'new' accordingly.

etimberg

Looks good. Tested it out and it worked nicely.

1 last thing for the sample. It should be added to https://github.com/chartjs/Chart.js/blob/master/samples/samples.js so that it appears on the sample page

etimberg · 2017-08-20T22:28:16Z

samples/scales/toggle-type.html

+    document.getElementById('randomizeData').addEventListener('click', function() {
+        type = type === 'linear' ? 'logarithmic' : 'linear';
+        window.myLine.options.title.text = 'Chart.js Line Chart - ' + type;
+        window.myLine.options.scales = {


@xg-wang I am ok with that stipulation as long as we add it to the documentation

etimberg · 2017-08-20T22:59:24Z

samples/scales/toggle-type.html

+    <div style="width:75%;">
+        <canvas id="canvas"></canvas>
+    </div>
+    <button id="randomizeData">Randomize Data</button>


[nit] We should rename this to "Change scale type" or something similar.

xg-wang · 2017-08-20T23:03:33Z

@etimberg Great! Should I also squash these changes to 1 commit? There are several reverts.

etimberg · 2017-08-21T00:15:40Z

@xg-wang squashing to one commit would be great. One last thing, if you could also update the documentation regarding update in https://github.com/chartjs/Chart.js/blob/master/docs/developers/updates.md to talk about what you mentioned in the comment above regarding creating a new object if the type changes.

xg-wang · 2017-08-21T06:29:49Z

docs/developers/updates.md

+```
+
+Scales can be updated separately without changing other options.
+It is recommended to update scales by creating a new object rather than mutating the properties to be changed, unless you are confident all scales options would update as you wish - especially when upating type of the scale.


Is this ok? Or is it better to just recommend update as a new object when changing type.

I think it's easier to just suggest creating a new object as needed. It will hopefully avoid some github issues later

updated with code snippet

(Needed to patch in chartjs/Chart.js#4198 In order to get chartjs/Chart.js#4939 Axis type switching to work)

* Put the url being tested on the page * Allow setting of log scale on either axis (Needed to patch in chartjs/Chart.js#4198 In order to get chartjs/Chart.js#4939 Axis type switching to work) * Allow the setting of min on X axis too Ignore 0 for min when in log * Smaller chart.js with all the fixes 0 log is ok now

ldemailly · 2017-11-29T21:15:16Z

hey can we get this in ? what is holding it up ?

xg-wang · 2017-11-29T21:19:48Z

@ldemailly Sorry I've no idea.. @simonbrunel could you review this code?

ldemailly · 2017-11-29T21:33:17Z

it's already accepted by 1 person but maybe you need to address the codeclimate lints ?

simonbrunel · 2017-11-29T21:35:52Z

@xg-wang sorry, really busy these last months, forgot about this PR. Reviewing right now!

The CodeClimate issues was here before and don't need to be fixed.

simonbrunel

Looks good, thanks @xg-wang. We definitely need to get rid of this scale "arrays" and support a single map of scales instead. That would simplify everything, but I guess it needs to wait until v3.

config: {
  scales: {
    foo: {  // the scale id
      type: 'time',  
      position: 'bottom', // implicit x-axis
      // ...
    }
  }
}

xg-wang · 2017-11-29T21:56:36Z

Thanks @simonbrunel . BTW what would be the expected time for V3? Looking forward to it.

simonbrunel · 2017-11-29T22:09:55Z

Nothing planned since we are not working on a v3 and we don't want to release it with APIs that we don't like. The idea (discussed with @etimberg) is to cleanup the v2 API to get really close to what we envision for a v3. That means we will deprecate stuff progressively, continue to introduce new features, then switching to v3 would basically consist in removing deprecated code and add features that are not possible in v2 (e.g. changing the scales options format would be quite complicated in v2 without breaking changes)

benmccann · 2017-12-02T22:18:47Z

@simonbrunel @etimberg would you guys be able to create a milestone in github for 3.0? Then we could file an issue to track stuff like this that we might want to consider for v3 so that we don't lose track of it

etimberg · 2017-12-02T22:34:57Z

Done @benmccann

benmccann · 2017-12-02T23:44:04Z

Great! Thanks!

- allow options to be updated in-place or as a new object - re-merge new options and rebuild scales & tooltips - preserve reference to old scale if id/type not changed - related tests and new sample also added. - update document about options update - update doc and example

Cached plugin descriptors hold a reference on the plugin options, which break if the plugin options object is replaced. That case happens when the user updates the plugin options with a new object, but also during since the new config update logic (chartjs#4198) that now always clone the plugin options. The fix consist in explicitly invalidating that cache before updating the chart.

Cached plugin descriptors hold a reference on the plugin options, which break if the plugin options object is replaced. That case happens when the user updates the plugin options with a new object, but also since the new config update logic (chartjs#4198) that now always clones the plugin options. The fix consists in explicitly invalidating that cache before updating the chart.

Cached plugin descriptors hold a reference on the plugin options, which break if the plugin options object is replaced. That case happens when the user updates the plugin options with a new object, but also since the new config update logic (#4198) that now always clones the plugin options. The fix consists in explicitly invalidating that cache before updating the chart.

- allow options to be updated in-place or as a new object - re-merge new options and rebuild scales & tooltips - preserve reference to old scale if id/type not changed - related tests and new sample also added. - update document about options update - update doc and example

Cached plugin descriptors hold a reference on the plugin options, which break if the plugin options object is replaced. That case happens when the user updates the plugin options with a new object, but also since the new config update logic (chartjs#4198) that now always clones the plugin options. The fix consists in explicitly invalidating that cache before updating the chart.

etimberg requested changes Apr 29, 2017

View reviewed changes

xg-wang mentioned this pull request Apr 29, 2017

[BUG] scales undefined when update options with a new object #4197

Closed

simonbrunel added the type: bug label May 8, 2017

simonbrunel added this to the Version 2.7 milestone May 8, 2017

etimberg reviewed Aug 10, 2017

View reviewed changes

xg-wang force-pushed the fix/scale_options_update branch from 7a42827 to 3557635 Compare August 13, 2017 17:55

simonbrunel removed this from the Version 2.7 milestone Aug 13, 2017

xg-wang commented Aug 14, 2017

View reviewed changes

etimberg reviewed Aug 20, 2017

View reviewed changes

etimberg requested a review from simonbrunel August 20, 2017 12:14

xg-wang commented Aug 20, 2017

View reviewed changes

etimberg approved these changes Aug 20, 2017

View reviewed changes

xg-wang force-pushed the fix/scale_options_update branch from 739635a to 62098c2 Compare August 21, 2017 06:15

xg-wang commented Aug 21, 2017

View reviewed changes

xg-wang force-pushed the fix/scale_options_update branch from 62098c2 to d0ea25a Compare August 22, 2017 02:03

ldemailly mentioned this pull request Nov 11, 2017

[BUG] changing axis type (linear <->logarithmic) doesn't change on chart.update() #4939

Closed

ldemailly added a commit to fortio/fortio that referenced this pull request Nov 11, 2017

Allow setting of log scale on either axis

a1555f2

(Needed to patch in chartjs/Chart.js#4198 In order to get chartjs/Chart.js#4939 Axis type switching to work)

ldemailly mentioned this pull request Nov 11, 2017

Ability to fix X axis min,max and switch to log on both axis fortio/fortio#17

Merged

update doc and example

af3cb9d

simonbrunel approved these changes Nov 29, 2017

View reviewed changes

simonbrunel merged commit 333f2eb into chartjs:master Nov 29, 2017

xg-wang deleted the fix/scale_options_update branch November 29, 2017 21:56

simonbrunel mentioned this pull request Jan 6, 2018

core.controller.js bug breaking chartjs-annotation and chartjs-draggable plugins (configMerge issue) #5111

Closed

simonbrunel mentioned this pull request Jan 13, 2018

Fix updating plugin options #5144

Merged

etimberg mentioned this pull request Jan 18, 2018

Dynamically updating ticks.mirror not working since 2.6 #5129

Closed

jcopperfield mentioned this pull request Jan 24, 2018

[BUG] Can't update scale.ticks.* options at runtime since 2.7.0 #4896

Closed

kurkle mentioned this pull request May 6, 2019

Remove duplicate scale building #6247

Merged

snyk-bot mentioned this pull request Mar 9, 2020

[Snyk] Upgrade chart.js from 2.7.1 to 2.9.3 savinovvu/reg-exp#5

Closed

snyk-bot mentioned this pull request Apr 1, 2020

[Snyk] Upgrade chart.js from 2.7.2 to 2.9.3 baophucct/wallet#4

Open

fix scale options update error #4198

fix scale options update error #4198

Conversation

xg-wang commented Apr 29, 2017

etimberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etimberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xg-wang Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

xg-wang commented Aug 10, 2017

etimberg commented Aug 11, 2017

xg-wang commented Aug 13, 2017

xg-wang commented Aug 13, 2017

simonbrunel commented Aug 13, 2017

xg-wang commented Aug 13, 2017

xg-wang commented Aug 13, 2017

xg-wang left a comment

Choose a reason for hiding this comment

etimberg commented Aug 14, 2017

xg-wang commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xg-wang commented Aug 20, 2017

rebuild scales every time

new

update

etimberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xg-wang commented Aug 20, 2017

etimberg commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldemailly commented Nov 29, 2017

xg-wang commented Nov 29, 2017

ldemailly commented Nov 29, 2017

simonbrunel commented Nov 29, 2017

simonbrunel left a comment

Choose a reason for hiding this comment

xg-wang commented Nov 29, 2017

simonbrunel commented Nov 29, 2017

benmccann commented Dec 2, 2017

etimberg commented Dec 2, 2017

benmccann commented Dec 2, 2017

xg-wang Aug 10, 2017 •

edited

Loading