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

Ability to reset the instances id counter #6010

Closed
reksc opened this Issue Jan 23, 2019 · 6 comments

Comments

3 participants
@reksc
Copy link

reksc commented Jan 23, 2019

Feature Use Case

I have SPA and a collection of charts inside a view. My implementation of displaying some additional data on the chart canvas is based on the chart instance id (available e.g. inside animation.onComplete callback) , which should be in sync with the the data source array.

This works great until I have to redraw the view - all chart id's will still increment from internal counter, so my first chart can have an id > 0.

chartInstance.destroy() doesn't help, and it's clearly due to how it's implemented. ID's are generated by Chart.helpers.uid() method, which has var id inside a closure. I hacked it by redefining the uid method when I need to reset the counter, but... yeah.

Possible Implementation

Without rewriting the whole helpers object, perhaps the uid() function could have an optional param to reset the id value?

@kurkle

This comment has been minimized.

Copy link
Collaborator

kurkle commented Jan 23, 2019

Just out of curiosity, why don't you save the chart instances you create? (for example in an array, you can use the array index instead of id if that's really what you need).

@reksc

This comment has been minimized.

Copy link
Author

reksc commented Jan 23, 2019

As for saving the instances - I use Vue and have another layer of abstraction over chartjs called vue-chartkick.

I have all of the extra logic inside chartInstance.options.animation.onComplete callback. Inside the callback, you get the instance bound to this.chart. I can't think of any easy way to access the index other then iterating through instances and comparing the objects, which sounds like an overkill :)

@kurkle

This comment has been minimized.

Copy link
Collaborator

kurkle commented Jan 23, 2019

So you look up the data you need to display by the id? Canvas is accessible from this.chart.canvas. You could also do something like adding the id you need in chart options config.options.instanceId = yourId and access that from this.chart.options.inscanceId.

I'm not against resetting the id, just a bit worried - because its supposedly unique id and resetting it could cause unexpected results in some cases.

@reksc

This comment has been minimized.

Copy link
Author

reksc commented Jan 23, 2019

@kurkle thanks, just checked and turns out you can extend the options object without limits. I went through multiple questions regarding "how to pass additional data to chart" and the answers proposed defining a custom chart type or reference the data outside the chart scope. This looks like the easiest approach for many use-cases.

@reksc reksc closed this Jan 23, 2019

@simonbrunel

This comment has been minimized.

Copy link
Member

simonbrunel commented Jan 23, 2019

I'm quite against this proposal because you should not consider the uid() result as a sequential array index. For example, we could decide (for whatever reason) to change our uid() generator to return a UUID. In this case, "resetting" the generator would have no sense.

It should be relatively easy to implement your own mapping, for example by storing your chart uids in an separated array:

var chart = new Chart( ... )
my_chart_instances[data_index] = chart.uid;

// later ...

var uid = my_chart_instances[data_index];
var chart = Chart.instances[uid];
@reksc

This comment has been minimized.

Copy link
Author

reksc commented Jan 23, 2019

@simonbrunel Makes sense. My confusion was caused by the fact, that after destroying the view I had an empty object under Chart.instances - so it almost looked like a "fresh start" and I expected to have the same output as during the first iteration.

As I don't have the access to Chart instance creation (which is handled by a 3rd party library), passing everything I need inside the options object sounds like the best option I wasn't aware of before. As Chart.instances is an object, not an array of objects (probably for an easier access by id), it isn't suitable for index : id mapping - just want to make clear if one would think that this might be an option as well :).

Thank you for the detailed answer!

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