ckeditor / ckeditor5-react

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

CkeditorContext onReady function called before editors attach to context #513

Closed glynam1 closed 3 weeks ago

glynam1 commented 1 month ago

This seems to have been introduced in 7.0.0

When using the ckeditor react context demo, attempt to log the editors after onReady is called:

            <CKEditorContext
                context={ ClassicEditor.Context as any }
                contextWatchdog={ ClassicEditor.ContextWatchdog as any }
                onReady={context => {
                    console.log( context.editors.length );
                }}
            >

This will log out as 0.

Based on these findings onReady is being called too early, as functionality that relies on this behaviour to populate the list of editors in the context will break.

This essentially breaks our setup and prevents us from upgrading, as bar listening on every onReady callback, and then populating a list based on that, there is no way to populate this

Mati365 commented 1 month ago

Hi! Thanks for report. I'll check this.

Mati365 commented 1 month ago

@glynam1 Good catch! It looks like our React integration docs are a bit outdated, and we'll update them. Basically, onReady behavior has been changed after fixing race conditions in our integration introduced with the newest React versions.

At this moment onReady is called after initialization of context and after that we attach editors to it. So if you want to store editors in some kind of state, you can do:

import type { CollectionChangeEvent, Editor } from 'ckeditor5';

..

<CKEditorContext
  context={ ClassicEditor.Context as any }
  contextWatchdog={ ClassicEditor.ContextWatchdog as any }
  onReady={ context => {
    context.editors.on<CollectionChangeEvent<Editor>>( 'change', ( ev, data ) => {
      console.info( data.added, data.removed );
      // or
      console.info( context.editors );
    } );
  } }
>
 ..
</CKEditorContext>
Mati365 commented 1 month ago

I'm closing this issue, please reopen it if the problem still occurs.

glynam1 commented 1 month ago

@Mati365 does an event emit when the context has reached a steady state? It seems like this is a change of behaviour, as onReady used to fire at the end of startup, whereas with the above snippet, the "end state" is unknown

Mati365 commented 1 month ago

@glynam1

does an event emit when the context has reached a steady state

Yep, but it does not mean that context has assigned any editor.

It seems like this is a change of behaviour, as onReady used to fire at the end of startup, whereas with the above snippet

Unfortunately, yes. The main reason for the changes was the recurring crashes of the integration in the latest nightly versions of React (stable Next.js was also affected) in StrictMode. Basically, there were no guarantees that the result of onReady callback returned correct instances of editors because in small amount of cases editors were initialized on incorrect instance of context. It's described here https://github.com/ckeditor/ckeditor5-react/pull/480.

the "end state" is unknown

It's known. change event is fired several times during startup process of nested editors, so if 3 of 3 editors are present in context, then the end state is reached.

glynam1 commented 1 month ago
It's known. change event is fired several times during startup process of nested editors, so if 3 of 3 editors are present in context, then the end state is reached.

I see, we will need to manage an expected number of events to fire and cross reference that with the callbacks. This changes quite a bit our implementation.

May I ask that we call these kinds of changes out in releases in future? This caused quite a bit of time in our team for investigating what has gone wrong and whether it was related to the new install setup

Mati365 commented 1 month ago

May I ask that we call these kinds of changes out in releases in future? This caused quite a bit of time in our team for investigating what has gone wrong and whether it was related to the new install setup

@glynam1 Thanks for letting us know. Sorry about that. We’ll make sure to highlight these kind of changes in future releases.

glynam1 commented 1 month ago

Hi @Mati365 ,

I see further issues when trying to implement this, it seems that the editors get added to the context before they've initialised any plugins

            <CKEditorContext
                context={ ClassicEditor.Context as any }
                contextWatchdog={ ClassicEditor.ContextWatchdog as any }
                onReady={(context, editors ) => {
                    context.editors.on('change', () => {
                        console.log('in callback');
                        context.editors.first?.plugins.has( 'Essentials'); //throws because plugins is not defined
                    });
                }}
            >

Whereas

                <CKEditor
                    editor={ ClassicEditor as any }
                    data="<h2>Another Editor</h2><p>... in common Context</p>"
                    onReady={ ( editor: any ) => {
                        window.editor1 = editor;
                        console.log(editor.plugins.has( 'Essentials' ));
                        setState( prevState => ( { ...prevState, editor2: editor } ) );
                    } }
                />

Seems to work fine

At runtime pretty much everything off the "new" editor is undefined using the callback from the context

Is this expected?

Mati365 commented 1 month ago

@glynam1 Expected, but not super intuitive. Consider using: editor.once( 'ready', () => {} ). We'll discuss internally adding shorter callbacks to this component to make it more intuitive.