chartjs / Chart.js

Simple HTML5 Charts using the <canvas> tag
https://www.chartjs.org/
MIT License
63.93k stars 11.89k forks source link

Passing a number value as a string causes the chart to silently fail #796

Closed grofit closed 8 years ago

grofit commented 9 years ago

I have a scenario where the chart interacts with knockoutjs, and unfortunately when the users input is bound to the data behind the scenes it often can be put in as a textual value not a numeric value.

There is an argument for me sanitising my values before hand to make them numeric doing 0 + textualNumber or something, however in this case its a dynamic chunk of data I am being passed and I dont know what data should be text and what should be a number, so would it be a use case you guys would support?

http://jsfiddle.net/jobofpj1/2/

The above link shows this happening, on your side you know what bits of the json you expect to be numbers etc, anyway I leave it up to you if you want to support or not.

moe-khan123 commented 9 years ago

I've come across the same, what I did in the Chart.js file is I modified the code and added this wherever I found segment.value. Not sure if this is the best way though but it works for me. It simply parses the the string to an integer if the value coming in is a string...

                    var valueToShow; 
        if (typeof segment.value == 'string' || segment.value instanceof String  )
        {

            valueToShow= parseInt(segment.value);
        }
        else
        {
            valueToShow = segment.value;
        }
grofit commented 9 years ago

I did a fork where I added a quick data normalizing step before the data was used, but then that would require subsequent custom chart types to adhere to it which is not realistic. Then I thought about just fixing it in the specific broken points, as some areas allow textual numbers others do not, so I started looking into that, then just ended up fixing it my side.

saintedlama commented 9 years ago

It's not the chart components responsibility to clean up the data series fed into the charting component, it's in the responsibility of the caller. If you really want this to be realized in the charting component you could extend Chart.js quite easily. Example for Line charts

Chart.types.Line.extend({
    name : "NormalizingLine",

    initialize : function(data) {
      // Iterate over data.dataSets here and remove all non string values...

      Chart.types.Line.prototype.initialize.apply(this, arguments);
    }
 })
grofit commented 9 years ago

You are right, however given that javascript is not statically typed it should not be too far outside the realms of possibility for code that requires a number to either throw an error if it does not get one, or just handle textual numbers etc. There is of course a limit as to what you can code defensively before everything falls over, and from what I saw most of ChartJS handled textual numbers fine, it was just certain bits which did not, like the animation I think blew up when it had a number as a textual as it was doing some math on it.

Anyway I have sorted the data on my side in this scenario, but it would be recommended to have some form of error not just a silent fail as it took me a while to figure out what the issue was (knockout providing user input as text not numbers). In most cases this would be transparent to the developer, so you would be scratching your head, even console.logs would not yield any useful information on this scenario, it was only when I debugged specifically on the data going over I noticed that one had a textual number.

Also as just a comment on previous fix ideas I think that it would be better to go with parseFloat opposed to parseInt as I assume chartJS can handle float data too so you dont want [ "1.2", "1.4", "2.4" ] getting truncated to [1, 1, 2]

etimberg commented 8 years ago

Just tested with v1.0.2 this appears to be fixed :) Please re-open if it still reproduces.