[Chart.js] Listen to Stimulus disconnect event to destroy the chart

[Shadow-Devil]

Q	A
Bug fix?	yes
New feature?	no
Issues	Fix #1408
License	MIT

Adds an extra check if the chart is already connected and short circuits, since otherwise creating the new Chart would throw an error.

[Shadow-Devil]

@smnandre will you be able to merge this or is something missing?

[smnandre]

@smnandre will you be able to merge this or is something missing?

Hi @Shadow-Devil! Sorry we've all been very busy on our "day lifes"..

II have a doubt here i'd like the opinion on others..

This situation (a chart already initialized on the element) should not happen, right ?

So i totally agree we should add a early exit there, but.... should we throw an error ?

@kbond @WebMamba

[Shadow-Devil]

@smnandre Sorry, I didn't want to sound rude😅.
I just thought this issue might have gone under the radar.

This situation (a chart already initialized on the element) should not happen, right ?

This situation happened for me, because of Google's Material Design Lite which would disconnect and reconnect the chart from the DOM.

A second solution would be to call this.chart.destroy() when disconnecting the chart.

[Kocal]

Hey, either throwing the error or exiting silently won't really fix the initial issue right?

Throwing an error looks better to me, because it will really help the developer to understand that something wrong is happening.

But, if the Chart is previously initialized and connect() is called a second time, to me it's a not-wanted scenario, the Chart should NOT exists yet when connect() is called. I believe this should be fix at the root, destroying the Chart on controller disconnect().

WDYT?

[smnandre]

because of Google's Material Design Lite which would disconnect and reconnect the chart from the DOM.

Everytime, or in particular scenarios ? It feels a bit strange :|

--

But, if the Chart is previously initialized and connect() is called a second time, to me it's a not-wanted scenario, the Chart should NOT exists yet when connect() is called.

That's my opinion too: in this scenario, throwing an Error after an early exit seems the correct thing to do (this PR + an error)

--

A second solution would be to call this.chart.destroy() when disconnecting the chart.

I believe this should be fix at the root, destroying the Chart on controller disconnect().

This feels a good thing to do yes! (there may be a reason why it has not be done before ( 🤷 ))

(thanks @Kocal)

[Shadow-Devil]

Everytime, or in particular scenarios ? It feels a bit strange :|

Once on page reload.

But, if the Chart is previously initialized and connect() is called a second time, to me it's a not-wanted scenario, the Chart should NOT exists yet when connect() is called.

The connect method can be called multiple times by stimulus. See also the stimulus docs about lifecycle callbacks:

A disconnected controller may become connected again at a later time.
When this happens, such as after removing the controller’s element from the document and then re-attaching it, Stimulus will reuse the element’s previous controller instance, calling its connect() method multiple times.

I will try to update the PR to destroy the chart on disconnect 👍🏻

[Kocal]

A disconnected controller may become connected again at a later time.
When this happens, such as after removing the controller’s element from the document and then re-attaching it, Stimulus will reuse the element’s previous controller instance, calling its connect() method multiple times.

Didn't know about that, thanks :)

[smnandre]

The connect method can be called multiple times by stimulus. See also the stimulus docs about lifecycle callbacks:

Only after beeing deconnected. So if the Chart.js instance is destroyed this would not create problem right ?

[Shadow-Devil]

Hm, so calling destroy in the disconnect works for my scenario, but now it has a little "stutter" since it stops the rendering and rerenders it again.
I think a better solution would be to not use connect but initialize and do all of the setup in there. This would only be called once and we don't have this creation/deletion problem. (Docs)
What do you think?

[smnandre]

In your case i understand this can be not perfect... (but still don't understand why your framework deconnect then reconnect div on page load)

But to me, it seems logical to use connect there, because it can happen multiple times.

That makes me think.... is this not the reason there is no disconnect: https://ux.symfony.com/demos/live-component/chartjs

Would this demo still work if we did disconnect every time? (genuine question).

Let's continue tomorrow !

[Shadow-Devil]

Would this demo still work if we did disconnect every time? (genuine question).

Pretty sure yes, since otherwise you would run into the same issue as me.
There is an overall logic error IMO: Stimulus can call the connect method multiple times, but the code inside it will throw an error if it is called a second time (without destroying the chartjs instance in between).

[smnandre]

You're right @Shadow-Devil!

I'm very sorry this MR took so long.

Made a minor comment, let's fix this, please rebase + add an entry in the CHANGELOG.md file (next release will be 2.23)
... and let's merge this :)

[github-actions]

📊 Packages dist files size difference

ℹ️ No difference in dist packagesFiles.

[Shadow-Devil]

Hi @smnandre I rebased the PR, added your review suggestion and added an entry in the CHANGELOG file.

Please let me know if there is anything else you need from me :)

[smnandre]

Thank you very much for your patience @Shadow-Devil

[Kocal]

Thank you @Shadow-Devil.