Wrap popular parts of the code into try-catch block

[pjasiun]

We could wrap some popular places where error can occur in the try-catch to replace a generic exception to the CKEditor 5 specific one. Such specific error could be later caught to restart the editor.

Some places we could add such try-catch blocks:

• engine.model.change,
• engine.view.change,
• engine.transformSets,
• core.command.execute,
• engine.view.observer events.

[ma2ciek]

Firstly I'll check if the try-catch block in such places won't cause any performance leaks.

[ma2ciek]

I've checked it and try-catch blocks shouldn't be a problem in terms of performance.

[pjasiun]

❤️

[pjasiun]

I think that we should also make sure that all of the async code is wrapped (promises has catch block which wraps errors in CKEditor error class).

[ma2ciek]

Do we have promises somewhere except the editor initialization?

[pjasiun]

Upload adapter? You can look for Promices, and addeventlistener. @Reinmar any more ideas?

[Reinmar]

There are some native listeners added here (via DomEmitterMixin): https://github.com/ckeditor/ckeditor5-widget/blob/master/src/widgetresize.js#L44-L76

Also, I'd expect to find some things via searching for addEventListener.

But most of those things are not worth covering. In fact, an error in such a listener usually doesn't crash the app.

From things that I know that were causing issues I can mention the upload stuff. There are promisses there and a quite complicated logic that caused some issues for us recently. But here... touching these parts without analysing the logic can actually break them unless you're considering throwing from catch() to not stop the error. We've done some unfortunate fine tuning with @jodator there recently and we weren't happy with the overal state of that code, so be careful :).

[Reinmar]

There's also a huge list of listeners in the UI. But I guess this is also beyond the things that are worth watching.

[ma2ciek]

I have some doubts about the implementation.

First I was thinking about something like this:

try {
	// Some CKEditor 5 code.
} catch ( err ) {
	if ( err.is && err.is( 'CKEditorError' ) ) {
		throw err;
	}

	throw new CKEditorError( 'model-change-unexpected-error', this, {
		originalError: {
			message: err.message,
			stack: err.stack,
			name: err.name
		}
	} );
}

But now I feel this approach would increase the code size significantly if we implement these try-catches in many places (in our observers, mixins, etc). So maybe we could introduce a generic helper that would wrap code into the try-catch and throw a generic error?

It could be called watchForUnexpectedErrors( code, context ) or something similar, maybe you have some better ideas?

[pjasiun]

In fact, the only part which changes is the message so maybe:

watchForUnexpectedErrors( code, errMessage, context );

Or, alternately, we can take the message from err.message. The point of these try-catch blocks is not to have nice messages for unknown errors, but only to wrap errors.

[pjasiun]

On the second thought, I think that overwriting error messages may hide the real reason of the error and make the console less readable, so I think we should not overwrite the error message, and your proposal watchForUnexpectedErrors( code, context ) is fine for me.

[Reinmar]

I'm not sure if it should be watch*, because that implies that it will accept a callback and that's unnecessary.

I'd propose e.g. CKEditorError.rethrowUnexpectedError( ... ), to be used like this:

try {
	// Some CKEditor 5 code.
} catch ( err ) {
	CKEditorError.rethrowUnexpectedError( err, context );
}

Other possible method names could be related to "enhancing" or "rethrow as", etc.

[ma2ciek]

I'm not sure if it should be watch*, because that implies that it will accept a callback and that's unnecessary.

I'd propose e.g. CKEditorError.rethrowUnexpectedError( ... ), to be used like this:

I'm fine with this approach too and the name fits well.