ckeditor / ckeditor5-react

Official CKEditor 5 React component.
https://ckeditor.com/ckeditor-5
Other
426 stars 100 forks source link

Quick rerendering causes editor issues #473

Closed Mati365 closed 6 months ago

Mati365 commented 7 months ago

Suggested merge commit message (convention)

Fix: Add CKEditor component initialize / destroy semaphore to prevent race conditions during fast rerender. Closes https://github.com/ckeditor/ckeditor5-react/issues/442, https://github.com/ckeditor/ckeditor5-react/issues/469, https://github.com/ckeditor/ckeditor5-react/issues/471, https://github.com/ckeditor/ckeditor5-react/issues/476.


Additional information

Probably related to these issues: https://github.com/ckeditor/ckeditor5-react/issues/469 https://github.com/ckeditor/ckeditor5-react/issues/409 https://github.com/ckeditor/ckeditor5/issues/15980 https://github.com/ckeditor/ckeditor5-react/issues/442 https://github.com/ckeditor/ckeditor5-react/issues/476

glynam1 commented 7 months ago

Screenshot 2024-04-22 at 23 44 18

Maybe I'm misunderstanding the aim of this PR ( or its implementation )

But will rerendering editors during instantiation still cause CkeditorContext editors to crash with the above error?

Mati365 commented 7 months ago

@glynam1 Hmm, I haven’t come across this one before, and it appears that it hasn’t been tested in our coverage. Has there been a reported issue that details the crash? In my opinion, it is in scope of this PR, but it's hard to tell if it is fixed without seeing minimal reproduction info.

glynam1 commented 7 months ago

I had left a comment on this ticket: https://github.com/ckeditor/ckeditor5/issues/15980 as I had hoped that it would have fixed things. It appears that the Context plugin is often left out of these tests (you can't disable it either as per https://github.com/ckeditor/ckeditor5-react/issues/409)

Mati365 commented 7 months ago

@glynam1 Thank you, I'll check that.

DawidKossowski commented 7 months ago

@Mati365, I've noticed that we have a similar issue with the useMultiRootEditor hook. When you use demo-multiroot-react-18, enabling StrictMode and passing disableWatchdog: true into the props, and initializing asynchronously (for example, by passing the cloudServices config), you may observe that sometimes it falls into an infinite loop. I've checked, and it seems to be connected with the _destroyEditor function.

Let's check and fix it in this PR since it is connected to the same issue.

glynam1 commented 6 months ago

@Mati365 thanks for your work on this. Did you get a change to test the context editor for this? Just wondering should I look to upgrade when this gets released. Thanks

Mati365 commented 6 months ago

@glynam1 I checked our internal test suite and it worked fine. imo it might really depend on your testing library set, so I'm not sure if my current solution solves your issue. You have to check.

glynam1 commented 6 months ago

Cool thanks, I'll open a ticket if I see the error reoccurring after this upgrade.