Fixed watchdog crash after immediately refreshing application

[DawidKossowski]

Suggested merge commit message (convention)

Fix (watchdog): Watchdog will no longer crash after immediately refreshing the application. Closes #15980.

---

Additional information

Fixes: ckeditor/ckeditor5-react#370.

[scofalik]

Hm, do I understand correctly that now create()s are chained. So one waits for the another one before it starts? And, is it the same in ContextWatchdog?

If so, I don't know if that will not hurt some of our integrators. I remember that in some cases, when using multiple editors, some of our integrators were creating editors in parallel and it sped up total loading time nicely.

[DawidKossowski]

I checked it, and ContextWatchdog creates an independent EditorWatchdog instance for each "editor". Therefore, my logic does not block the parallel initialization of multiple editors within the context of ContextWatchdog.

[oleq]

I took the liberty of pushing small code refactoring that reduces magic a little bit here (it took me a while to get my head around these changes 🙂)

One note, though: The create() test introduced in this PR is weak

ckeditor5/packages/ckeditor5-watchdog/tests/editorwatchdog.js

Line 215 in 45a1780

it( 'should create an editor instance after the ongoing destruction process has been finished', async () => {

. It passes when you

diff --git a/packages/ckeditor5-watchdog/src/editorwatchdog.ts b/packages/ckeditor5-watchdog/src/editorwatchdog.ts
index 0919c38968..64f270b3b5 100644
--- a/packages/ckeditor5-watchdog/src/editorwatchdog.ts
+++ b/packages/ckeditor5-watchdog/src/editorwatchdog.ts
@@ -257,7 +257,7 @@ export default class EditorWatchdog<TEditor extends Editor = Editor> extends Wat
                   config: EditorConfig = this._config!,
                   context?: Context
          ): Promise<unknown> {
-                  this._lifecyclePromise = Promise.resolve( this._lifecyclePromise )
+                  this._lifecyclePromise = Promise.resolve()
                            .then( () => {
                                     super._startErrorHandling();

and this has nothing to do with my refactoring (I checked before and after). Maybe there's a better way to write this test? You could listen to stateChange and see the #state and then make some spy call order assertions maybe.

The destroy() test does its job, though.

On a side note: an integration test in ckeditor5-react would be lovely too. We can rewrite this code in the future and break the business logic for outsiders again and we wouldn't even realize until bug reports start arriving to ckeditor5-react.

[DawidKossowski]

@oleq, first of all, I was hoping for comments instead of direct changes. 😞 It would have been better for me and could have sparked some constructive discussion. Remember that you can make changes that might be incorrect, and the author may disagree with them.

Anyway, I will respond to your changes:

1. You changed the variable name from _editorProcessing to _lifecyclePromise. Firstly, I wanted to name it similarly to you, but I wanted to keep it focused on the functionality. Maybe your proposal is better, maybe not. But, you pushed the changes...
2. I wanted to keep the variable currentProcessing since this approach ensures clarity of intention by explicitly stating that the current value of this._editorProcessing is stored in the variable currentProcessing for later use. By employing this method, we enhance the code's readability and make it easier for other developers to understand. Additionally, debugging becomes simpler as it allows for tracking and analyzing the state of this._editorProcessing at specific points in the code. But, we can stay with your proposal.
3. You moved _lifecyclePromise directly to Promise.resolve. I can trust you that you checked and thought about it, but I need to make sure. Let's consider 2 different examples.

The first one:

const promise = Promise.resolve( () => { 
  throw new Error('Error occurred')
} );

promise
  .then( () => {
    console.log( 'Success' ); // It will be processed.
  } )
  .catch( error => {
    console.error( 'Error caught:', error.message ); // This catch will not catch the error.
  } );

And the second one:

const promise = Promise.resolve()
  .then( () => {
    throw new Error( 'Error occurred' );
  } );

promise
  .then( () => {
    console.log( 'Success' );  // It won't be processed.
  } )
  .catch( error => {
    console.error( 'Error caught:', error.message ); // This catch block will handle the error.
  } );

Are you sure that we don't need the benefits arising from the second approach? I'm not entirely convinced, which is why I preferred the previous form.

4. When it comes to unit tests, you are right. I will enhance it.

[oleq]

@oleq, first of all, I was hoping for comments instead of direct changes. 😞 It would have been better for me and could have sparked some constructive discussion. Remember that you can make changes that might be incorrect, and the author may disagree with them.

I read your code and I couldn't get it for a while. This was the first time I'd seen anything like it and I realized that you coded it this way because you missed out on an opportunity to pass the ongoing promise to Promise.resolve(). I corrected that because IMO an elegant promise chain should be coded like this.

As for your concerns regarding error handling, I verified that

const firstPromise = new Promise( () => {
    throw new Error( 'first promise error' );
} );

Promise.resolve( firstPromise )    
    .then( () => {
        console.log( 'after first promise' )
    } )
    .catch( error => {
        console.error( 'Error caught:', error.message ); 
    } );

catches errors as well as

const firstPromise = new Promise( () => {
    throw new Error( 'first promise error' );
} );

Promise.resolve()    
    .then( () => {
        return firstPromise;
    } )
    .then( () => {
        console.log( 'after first promise' )
    } )
    .catch( error => {
        console.error( 'Error caught:', error.message ); 
    } );

We can disagree on the name of the private property. IMO the "current" prefix is very good but "processing" is extremely vague (maybe less vague since we have TS, but still). I'm not attached to my change in this matter, though.

But, you pushed the changes...

I think we approach PRs differently 🙂 To me any PR (except epics) is a scratchpad and there's nothing wrong with pushing to it. It's a communal effort the moment the code gets public. Sometimes it's easier to make your point this way rather than to write numerous comments and/or paste diffs (or fork branches). You can keep working on top of what I did or revert my commit if you find it unsuitable, that's fine, I don't mind 🙂

[glynam1]

I would really like to see this merged. We seem to run into this issue when the user is navigating to and from editing sessions using useNavigate() from react router.

Despite cutting down on extra renders, its still easy to trigger by loading/unloading multiple docs in quick succession

[DawidKossowski]

@glynam1, we will release it in mid-April. If you still have any issues regarding Watchdog and React integration, let us know.