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

Vue reactivity hacky with reactiveData mixin #351

Closed
FossPrime opened this issue Apr 26, 2018 · 10 comments
Closed

Vue reactivity hacky with reactiveData mixin #351

FossPrime opened this issue Apr 26, 2018 · 10 comments

Comments

@FossPrime
Copy link

FossPrime commented Apr 26, 2018

The first value should update every second. When you hover over the data point, you see the real updated value. But the graph does not update.
https://codepen.io/fossprime/pen/YLGBNm

The easy fix is to copy the data to a new object, which is not computationally ideal. It's also just weird, not how Vue works.

Expected Behavior

Chart updates when the Vue data is changed, as most reactive things in Vue do

Actual Behavior

Chart does not update unless the dataset is replaced

Environment

  • codepen
@apertureless
Copy link
Owner

You should check out the Reactivity & Limitations section in the docs.

  1. Limitations of $watch
    https://vuejs.org/v2/api/#vm-watch

Note: when mutating (rather than replacing) an Object or an Array, the old value will be the same as new value because they reference the same Object/Array. Vue doesn’t keep a copy of the pre-mutate value.

  1. And the Change detection
    https://vuejs.org/v2/guide/reactivity.html#Change-Detection-Caveats

Due to the limitations of modern JavaScript (and the abandonment of Object.observe), Vue cannot detect property addition or deletion.

  1. Generall Caveats
    https://vuejs.org/v2/guide/list.html#Caveats

Due to limitations in JavaScript, Vue cannot detect the following changes to an array:

When you directly set an item with the index, e.g. vm.items[indexOfItem] = newValue
When you modify the length of the array, e.g. vm.items.length = newLength

@FossPrime
Copy link
Author

FossPrime commented Apr 30, 2018

I've updated the pen to better illustrate the issue. It is within the limitations of when using Vue's set to modify a value via index and still have it react, as I showed in the similarly structured message component in the pen, which does react, while vue-chartjs doesn't. I'm not sure what the fix is.

https://codepen.io/fossprime/pen/YLGBNm

The main point is, Vue definitely supports changing values deep in an object without trashing the object, what it doesn't like is creating objects/arrays via index, as opposed to using set, which is what point #1 means. The code in the pen changes a value and the other components with the exact same data react. This is further confirmed by the Vue.js inspector allowing you to increment values in chartData, to no effect. Point #3 can be achieved with set, which is not supported, but even replacing the data values array won't update the chart, the entire object must be trashed for it to update.

Not sure if Chart.js is built in a way that allows modifying one value without trashing an object... if it doest than adding proper reactivity would just be a developer creature comfort feature rather than a genuinely beneficial feature

This is probably an enhancement or bug, not a question, depending on to what extent Vue reactivity is claimed supported.

Fun fact, it will make a one time update if you resize the screen. That could lead to some hard to find bugs, it's not deterministic.

@apertureless
Copy link
Owner

Hey @rayfoss

well I guess the "problem" is that in your pen, you are mutating the data. Without changing either the length of the data array nor changing the labels.

The whole chart.js reactivity topic is quite hacky. Because chartjs is not reactive per default. You can mutate the data like you want but have to call chart.update() which will cause an update.

So in theory something like

watch: {
	chartData () {
        chart.update()
    }
}

should do the trick. However this is not working in every scenarios. There is also somewhere an old issue that goes deeper into it. As far as I remember chart.update() does not work if you add new datasets. Thats why the mixin got a ton of logic and comparing into it to check if the data is really the same or not. And then either call update() or do a full renderChart() call.


Regards to your pen, I guess if you would change labels, too it would work.


I will check on the current chart.js updates and see if they maybe fixed the update() method to work with adding datasets too and see if I can simplify the mixins and make them more robost.

However the question is also, which use case do you have, mutating individual values in the given dataset data. Normally you would add new values to the datasets[0].data array. At least I can't think of a scenario where mutation existing values would be useful.

@FossPrime
Copy link
Author

FossPrime commented May 2, 2018 via email

@apertureless
Copy link
Owner

Ah I see, yeah that might be a use-case if you have realtime data over a websocket.

@davidmichineau
Copy link

Hey !

WorkAround
Why not just do something like

watch : {
    deepArr () {
       this.render();
    }

and add method render

methods : {
    render () {
      this.renderChart(this.chartData, {
        responsive          : true, 
        maintainAspectRatio : false,
        animation           : { duration : 1 }
      })
    }
  }
}

@w3cj
Copy link

w3cj commented Jun 24, 2018

Correct me if I'm wrong, but it would seem that the way the reactiveData and reactiveProp mixin watch for changes is not a deep watch: https://github.com/apertureless/vue-chartjs/blob/develop/src/mixins/index.js#L75

So if anything is changed inside of chartData like datasets or datasets.data even using this.$set or Vue.set, the watcher will not be called.

According to the Vue.js documentation, you can add the deep: true property to a handler to watch nested properties. https://vuejs.org/v2/api/#watch

So the updated code in /src/mixins/index.js would be:

export const reactiveData = {
  data () {
    return {
      chartData: null
    }
  },

  watch: {
    'chartData': {
      handler: dataHandler,
      deep: true
    }
  }
}

export const reactiveProp = {
  props: {
    chartData: {
      required: true
    }
  },
  watch: {
    'chartData': {
      handler: dataHandler,
      deep: true
    }
  }
}

I would make a pull request myself with this change, but I'm not sure what implications it has for people already using this library. @apertureless Any thoughts on this?

EDIT: POC

To show this working, I published a version of this library with the above changes made to the watch in the mixin.

Here is @rayfoss example now working as expected: https://codepen.io/anon/pen/GGBKMg
Updating the first value in the data array causes the watch to be called and the chart updates.

This does however cause the following warning in the console:

You may have an infinite update loop in watcher with expression

Now that the watch is deep, the dataHandler might need to be updated to account for this.

@apertureless
Copy link
Owner

apertureless commented Jun 24, 2018

This would need some further testing.
The very first version had the deep:true prop set in the watcher.
However the watcher crashed often, because the object was simply to big.
And I got often memory exceeded errors.

@FossPrime
Copy link
Author

Well the exact original issue is no longer manifesting with 3.4.2...

@alanhyue
Copy link

This would need some further testing.
The very first version had the deep:true prop set in the watcher.
However the watcher crashed often, because the object was simply to big.
And I got often memory exceeded errors.

Intuitively deep:true causes infinite updates because deep down somewhere, this.renderChart(this.chartData, this.options); is mutating this.chartData. So the solution would be making a copy of the chart data and pass it to the method.

My final component is this, it is reactive on nested chartData prop:

<script>
import { Bar } from 'vue-chartjs';
import _ from 'lodash';

export default {
  name: 'BarChart',
  extends: Bar,
  props: ['chartData', 'options'],
  mounted() {
    // Overwriting base render method with actual data.
    this.renderChart(this.chartData, this.options);
  },
  watch: {
    chartData: {
      handler() {
        const clone = _.cloneDeep(this.chartData);
        this.renderChart(clone, this.options);
      },
      deep: true,
    },
  },
};
</script>

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

5 participants