dataDidChange fires twice

[andrewmtoy]

Hey @Glavin001, thanks for creating this library!

I noticed today that the dataDidChange method fires twice after initial render for all subsequent data attribute changes. It looks like you're registering the observer a second time in the didInsertElement method.

This has caused rendering issues with the C3 library because chart.load is invoked twice in quick succession.

What is the use case for notifying components of changes from the controller with 'notifyPropertyChange'? Couldn't we just pass in 'data' and trigger chart.load in a 'didReceiveAttrs' life cycle hook? I think a good fix for this bug would be to delete 'didInsertElement' entirely.

I have a bug demo repo set up here: https://github.com/andrewmtoy/ember-c3-bug-demo.

I believe that this pull request is solving this issue using the debouncer method.

Let me know what you think or if you'd like me to create pull request with these changes.

Thanks!
Andrew

[Glavin001]

Hi @andrewmtoy. Thanks for creating this issue. In your demo that reproduces the bug, have you tried and confirmed that #44 fixes this issue? If so, I will merge immediately.

There have likely been a lot of changes to Ember since this package was created. I think removing didInsertElement and update how things work internally makes sense. I would be happy to review and merge a Pull Request for this functionality. Thanks in advance!

[andrewmtoy]

#44 does not fix the duplicate observer being instantiated, but it does fix the C3 rendering issue I was seeing. I'll take some time this week and send over a pull request with the changes I mentioned.

[jsvg]

@andrewmtoy any update on this? would be able to take a second look if not

[andrewmtoy]

@jsvg, I'm going to take a shot at this tomorrow.

[andrewmtoy]

Hey @jsvg, @Glavin001. I took a look at using the new component lifecycle hooks in this addon. Here is my first attempt. https://github.com/andrewmtoy/ember-c3/tree/use-new-component-lifecycle-hooks

I've been using this in my project today, and everything seems to be working fine.

Let me know what you think. If you guys like it, I'll add tests and open a pr.

[jsvg]

This seems to solve my case for the time being!

[andrewmtoy]

@Glavin001, if you're cool with this, I'll add tests and open a pr.

[jsvg]

Actually @andrewmtoy, in my case I'm modifying chart parameters (axis, groupby, etc) with user input options. Updating these options requires different computed properties on the controller to update. Since these cps are dependent on different promises, which resolve at different times, the graph still renders before all the data is available so I still need the debouncer to provide a semblance of control...

That said, I did a total rewrite of the component. Compliant with current ember coding practices.

[Glavin001]

@jsvg your rewrite looks great! Very concise. I think a fresh start would be good, since there have been many changes, to Ember.js and with the availability of ES6, since Ember-C3 was first created.

I think the only thing I see that is missing is the Component's sendAction events: https://github.com/Glavin001/ember-c3/blob/master/addon/components/c3-chart.js#L173-L188

'oninit',
'onrendered',
'onmouseover',
'onmouseout',
'onresize',
'onresized'

@andrewmtoy & @jsvg : I would love to see a Pull Request that meets all of your needs and cleans up the code. I'm currently finishing up final exams for this semester and writing my thesis. So I likely will be more available starting in early May. Before then, I'll still do my best to review any Pull Requests that come my way. Thanks in advance and great work!

[maxwondercorn]

Issues should be resolved with prs #75 and #79. Updated version will published after checklist in issue #77 are completed