Error handling

[pjasiun]

We have log.warn and log.error utils: https://github.com/ckeditor/ckeditor5-utils/blob/master/src/log.js for a long time, and it's still unclear what they should do.

log.error should be called when something very bad happens and the editor will crash. Such situation could be handled: a plugin or integration could report the error or reinitialize the editor. To do it, it needs to know that the error occurred.

This is why these methods should fire event. The problem is, on what object they should fire that event. The third parameter should be passed with the context, object on which event should be fired.

On the other hand, we do not want to hide the error: it should be thrown anyway from the catch block, so there will be the stack trace in the console.

To let users handle the error we should add try-catch block in places we know that error can occur (for instance document.enqueueChanges). Another problem is that document knows nothing about the editor, so we need to propagate the event: editor can listen on the document and propagate error event.

[thedavidmeister]

my editor keeps crashing and it's really hard to debug atm :( an event would be handy

[Reinmar]

Related issues:

• https://github.com/ckeditor/ckeditor5-engine/issues/1158
• why does ckeditor complain with warnings in the console when the el is detached from the dom? #725

[pjasiun]

Problems

At the moment we have far too many errors handling mechanisms. What we have is:

• CKEditorError exception,
• standard Error exception usage in some places in the code,
• the log.error method,
• the log.warn method (often used incorrectly),
• _crash event (used in Letters to restart the editor in case of an error, but hides errors if the event is not handled),
• EnableEditingDebug plugin,
• ENGINE_DEBUG pre-compilation flag (which does not work correctly if EnableEditingDebug is not added).

Note that, there are 3 main cases error handling mechanism should solve.

Human-friendly feedback

We want to give nice, human-friendly feedback, for developers who are integrating the editor or creating plugins, as soon as they do something wrong. In most cases CKEditorError is a good choice for them. If there is an error they want to know about it as soon as possible.

Help for debugging

There is more than just information about the critical exception. Sometimes you need to be able to follow the application flow. Sometimes part of it. However, this information is still only important for developers. They want to be able to disable it for end users.

Provide stable editor for end-users

End-users want to have a stable application. That's it. For them, we might have some mechanism which will restart the editor in the case of a crash, but the error message is usually not important for them. They need a watchdog mechanism which will listen on global errors and restart the editor in case of an error, not just an error in the console.

Solution

My proposal is to simplify the whole mechanism and have just two tools.

CKEditorError expetion

They should replace all log.error executions, since none of the groups mentioned above, is interested in errors with no crash. Some warning we have now are also in fact errors and should be replaced by CKEditorError. If something critical happens we should inform the developer about it as soon as possible. For sure CKEditorError should also replace a standard throw Error and _crash event too.

To be able to restart the editor by the watchdog, CKEditorError needs a context to be able to identify which editor crashed. The context might be the editor instance, but also any custom object available in the editor, for instance model. Note that engine and a big part of the UI classes do not have a reference to the editor class, so we can not force the editor as the context.

Additionally CKEditorError should work well with promises. We need to figure out how to do it. We might need to wrap it in the setTimeout 0 if there is no other option.

Precompiler flags

The second mechanism can be precompiler flags. For instance:

yarn run build --debug engine

Should replace:

// @if DEBUG_ENGINE // console.warn( 'something is wrong' );

to:

/* @if DEBUG_ENGINE */ console.warn( 'something is wrong' );

(note that it should work fine with the sourcemap since the position of characters do not change)

I believe that by default some warnings should be enabled (like rendering infinite loop warning or UI rect-source-not-in-dom):

yarn run build

Should still replace:

// @if DEV_ENV // console.warn( 'important warning!' );

to:

/* @if DEV_ENV */ console.warn( 'important warning!' );

However, it should be possible to disable it using yarn run build --production.

It should be also possible to add new types of debugging code if needed.

Source code:

// @if DEV_ENV // console.warn( 'Dev env' );
// @if DEBUG_ENGINE // console.warn( 'Debug engine' );
// @if DEBUG_UI // console.warn( 'Debug UI );

yarn run build --debug engine,ui
->
/* @if DEV_ENV */ console.warn( 'Dev env' );
/* @if DEBUG_ENGINE */ console.warn( 'Debug engine' );
/* @if DEBUG_UI */ console.warn( 'Debug UI );

yarn run build --debug engine
->
/* @if DEV_ENV */ console.warn( 'Dev env' );
/* @if DEBUG_ENGINE */ console.warn( 'Debug engine' );
// @if DEBUG_UI // console.warn( 'Debug UI );

yarn run build
->
/* @if DEV_ENV */ console.warn( 'Dev env' );
// @if DEBUG_ENGINE // console.warn( 'Debug engine' );
// @if DEBUG_UI // console.warn( 'Debug UI );

yarn run build --production
->
// @if DEV_ENV // console.warn( 'Dev env' );
// @if DEBUG_ENGINE // console.warn( 'Debug engine' );
// @if DEBUG_UI // console.warn( 'Debug UI );

Note that this way not only the console can be used, but any code can be conditionally executed, even imports. Thanks to it, we may use anything we need in the debug mode and do not slow down the production code nor increase the size of the build after the minification.

This way engine debug tool should be added to make them work out of the box when --debug engine flag is set.

Finally, it should not preclude the possibility of using @error and attachLinkToDocumentation for exceptions and warnings enabled in DEV_ENV.

I do not see any reason to keep log.error and log.warn API. It was proposed 3 years ago, at the very beginning of the development, but we did not find good usage for it. However, it the middle of this refactoring we may realize that we need something to utilize some steps and might bring some logging utils back.

[Reinmar]

I agree with most of the things and discussed them with @pjasiun F2F, so I'll just focus on the changes I'd propose.

It doesn't make sense to have an option like --production. This should be the default mode.

In general, there should be the following levels:

1. Default (production) – we still throw all the errors when the app reaches an invalid state (e.g. an invalid param was passed to a certain function, indexes out of bounds, etc.). For that, we should use CKEditorError().

   Additionally, in the default mode, we still need to log certain DX-oriented warnings like the one for missing upload adapter. For that, we should use console.warn() (but perhaps there are use cases for console.error() too). This kind of issues should not throw errors, but instead, fail gracefully (usually by an early return, so without affecting the call stack).

2. CK_DEBUG (standard debugging mode) – this mode should be by default on in all our testing envs – in yarn manual, yarn test, etc.

   In this mode we should log some general warnings for potentially incorrect app state that we feel may save us some time in the future when debugging something. One of such things can be the infinite selection loop. You don't need that in the production (cause this situation doesn't mean that the app is broken, because we have the safety check there which stops the rendering), but it still means that something went wrong and, as a developer, you should know about this.

   Also, as mentioned, this mode will be on in all our testing envs, so those logs will also appear in the yarn test logs, on CI, etc. In fact, it'd be great if any console.warn()/error() calls during test execution would throw the test, so we'd automatically get notified about such cases.

   With CK_DEBUG I'd normally use console.warn(). console.error() means that it's something that should be logged in the production (so see the previous point).

   We should not use CK_DEBUG to log verbose (frequent) things, to avoid flooding the console. If you need info about something verbose see the next point.

3. Finally, we can have more specific CK_DEBUG_* flags (e.g. CK_DEBUG_ENGINE) for stuff like engine debug tools, OT debugging, observables debugging, etc, etc. It'd be good to document the list of those flags somewhere.

---

PS. The above means that I'd remove the log object completely.

[pjasiun]

The first part of this task is done, thanks to @ma2ciek, we have a context in all CKEditorError and ckeditor5-watchdog. Now we need to introduce precompilation debugging flags and remove log.warn and log.error.

[ma2ciek]

Actually, I'm not sure with removing log.warn() and log.error().

1. With log.warn() / log.error() it's easy to provide a detailed error message in the default mode for such warning as we do now. We won't be able to do it with console.warn() and console.error().
2. With log.warn() / log.error() it's easy to change the behavior of these methods inside them and throw an error in the CK_DEBUG mode.

Otherwise, IDK really how to implement the above using only the native console. And IMO the final result should be much cleaner if we write some magic ifs inside our logger only.

cc @pjasiun @Reinmar.

[pjasiun]

• With log.warn() / log.error() it's easy to provide a detailed error message in the default mode for such warning as we do now. We won't be able to do it with console.warn() and console.error().

Since there are no many cases (so far we have one case for warning in the production code, missing upload adapter), we can simply do:

console.warn( attachLinkToDocumentation( message ), data );

Note that for all warn/error available only CK_DEBUG mode we do not need these links.

2. With log.warn() / log.error() it's easy to change the behavior of these methods inside them and throw an error in the CK_DEBUG mode.

Note that we should not change them in the CK_DEBUG mode. We should throw only in our automatic tests. In manual tests, they should act in a normal way. I think that in unit tests we can simply overwrite console.warn and console.error to make sure that tests fail when one of these methods is executed in a test. However, throwing errors in automatic tests is a separate ticket, it should not be solved as a part of this issue.

[Reinmar]

I think I agree with both things that @pjasiun wrote. We can KISS now.

[ma2ciek]

I think that in unit tests we can simply overwrite console.warn and console.error to make sure that tests fail when one of these methods is executed in a test.

@pjasiun How you want to achieve it for all our tests? With a regexp replacement in pre-processing?

[pjasiun]

I think that in pre-processing we should be able to add code which will be executed before tests and will simply overwrite these properties (I mean, like console.warn = ourCustomFunction;).

[ma2ciek]

I think that in pre-processing we should be able to add code which will be executed before tests and will simply overwrite these properties (I mean, like console.warn = ourCustomFunction;).

With this approach, we need to create a webpack plugin that would access the bundle and inject such code that will affect every usage of console.warn/console.error. And actually, we already have plenty of console.warn warnings on the CI - https://travis-ci.org/ckeditor/ckeditor5 - that we'd have to fix within this change.

We could add a beforeEach() hook for all tests, but this smells bad to me as well.

[pomek]

we already have plenty of console.warn warnings on the CI

All of them come from Karma WebServer and I'm pretty sure that we don't want to change anything in external tools. Some of those logs can contain useful information for developers (e.g. when you specify invalid fields in configuration object).

[Reinmar]

And actually, we already have plenty of console.warn warnings on the CI - https://travis-ci.org/ckeditor/ckeditor5 - that we'd have to fix within this change.

Do you mean this?

09 07 2019 13:44:55.528:WARN [web-server]: 404: /foo.jpg
.09 07 2019 13:44:55.574:WARN [web-server]: 404: /foo.jpg
.09 07 2019 13:44:55.727:WARN [web-server]: 404: /foo.jpg

These are not warnings coming from console.log/warn/error on the client. These are logs coming from Karma's webserver (Node's process). They don't even necessarily go through console.* but that isn't important here.

I don't see any console log coming from the browser, so I think we can safely override it. Can be beforeEach(), can be via webpack. Perhaps there's even a plugin for Karma to do that.

[pjasiun]

What I should do with the incorrect situation?

Is it caused by the wrong editor integration?

Yes. Does it a common error, which many developers may meet at the early stage of integration and we are able to initialize the editor despite it?

Yes.

Use console.warn, for errors like missing upload adapter or incorrect name of a toolbar item. Developers experience is important and we want to show them the working editor as fast as possible. If the first thing they see is an error, they will not like it, even if it is their fault.

No.

Throw CKEditorError exception, for other wrong API usages or when a custom plugin does something wrong. Usually, the developer wants to know as soon as possible about a mistake and it does not matter if the editor will be working despite the error or not. If it is not a matter of "the first impression", we should not care about running editor if the integration is wrong.

No, the error is usually a result of the user action. May it happen in a correct situation?

Yes/Maybe.

Use // @if CK_DEBUG // console.warn, for cases like infinite loop waning or incorrect focus warning. Users can do nothing with the error. Even developers can do nothing with it. Debugging warnings should be displayed only for CKEditor 5 developers, for debugging purposes.

No, the situation is definitely incorrect.

Throw CKEditorError exception. Thanks to it Watchdog will be able to restart the editor.

[pjasiun]

I believe that the next step to close this issue is to turn CK_DEBUG mode on in all our development environments: manual tests, automatic tests, local docs build, etc. and add support for custom debugging logs like CK_DEBUG_ENGINE.