search

grofit / knockout.chart

A knockout binding to allow chart.js and knockout to work together for the greater good.
MIT License
25 stars 12 forks source link

Fix get property names on browsers #6

Closed bucktronic closed 5 years ago

bucktronic commented 5 years ago

The observable properties on the model variable were not being found on any browser other than IE 11.

grofit commented 5 years ago

Hey, what was the problem exactly? as the for(... in ...) should be compatible with all browsers back to ie6, also the library was generally tested and developed against firefox and chrome so it worked fine.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in

I have also just run the demo in chrome and there is no error. I dont really maintain the library much these days so if this fixes an actual issue you or others are getting I am happy enough to PR it, but I would at least like to vet the reasoning first.

bucktronic commented 5 years ago

Hi there, yeah during our testing we saw that the for wasn't enumerating any _latestValue properties on the observables. This looks to be because _latestValue is of type Symbol which is omitted by the for loop (see your link). In our testing this meant that when the observables were updated, the chart wasn't being refreshed. We have tested on Chrome v72, FF v64, Edge v41. Does the demo load then update the chart data?

grofit commented 5 years ago

Yeah the demo works fine, but there were not any symbols used anywhere, so if you are needing symbols for whatever reason and this change has no negative impact then I am happy to add it in, just wanted to verify the use case. While I am a bit weary of checking the browsers from JS you would just end up substituting that for existence checking of methods I guess, so if someone else feels that its bad they can offer another PR to make that nicer.

Thanks for your submission.