tinymce / tinymce-blazor

Blazor integration
MIT License
45 stars 13 forks source link

Current editor does not handle disposing properly when binded value is set and another editor is initialized #46

Closed nguyenhthai closed 2 years ago

nguyenhthai commented 2 years ago

Issue: This issue was originally reported in #15. However, the fix to that caused issue with editor toolbar not disposing properly in #43 and thus was removed.

Reproducible step (Repo here):

exalate-issue-sync[bot] commented 2 years ago

Ref: INT-2880

jscasca commented 2 years ago

The initial issue was fixed by removing all events when disposing of the instance which caused toolbar to not be removed when during disposing time.

I will be looking into this and hope to have a PR in soon.

nguyenhthai commented 2 years ago

I noticed this dispose pattern and feel like we can move the reference dispose after success calling the JS interop method. Not really sure how that will affect the object disposing from Blazor and the JS interop. Or I think a better solution would be to communicate back to Blazor once the JS module have completely destroy the editor and trigger the garbage collector.

https://github.com/tinymce/tinymce-blazor/blob/e712f406aa48c64fc568c1f94e5cff8f8483cca8/TinyMCE.Blazor/Editor.razor#L285-L299

jscasca commented 2 years ago

I think the main problem was that some events were still bound by the framework, which is what editor.off() was trying to fix.

I made #47 to address this

nguyenhthai commented 2 years ago

I think the main problem was that some events were still bound by the framework, which is what editor.off() was trying to fix.

I made #47 to address this

I agreed that there could the problem with some events but I still think the reference object should only be disposed after the success of the javascript call. I looked at several articles about blazor interop and most if not all are doing the reference dispose after the js call.

jscasca commented 2 years ago

Basically, the problem is that the events keep the dotnetref alive, so by getting rid of the bound events we deal with the obj reference. This should happen in the same thread so I don't think there should be a problem. I will try to find an edge case and see if there is any risk. Otherwise, yeah I can wait for the task to complete before disposing of the reference.

nguyenhthai commented 2 years ago

@jscasca Any update on the bug fix release?

jscasca commented 2 years ago

@nguyenhthai yes, sorry. The previous code was released earlier today.

We ended up keeping the disposal order in which the dotnet reference is disposed followed by the destroy method call in JS.

This will likely be addressed in the future