Ckeditor5: "CKEditor Unexpected error" wrapper makes errors unreadable

[Reinmar]

Good luck analysing where this issue occurred:

We need to improve this as this will be a big PITA.

[Reinmar]

Those links to sources don't work. Clicking them opens a new tab:

[ma2ciek]

Hm. The core of the problem seems to be displaying the \n character as the CKEditorError JSON.stringify( data ) creates \\n from \n. Maybe we can add use something like JSON.stringify( data ).replace( /\\n/g, '\n' ) )? I can also make the beginning more readable.

[Reinmar]

Hm... IDK. It requires some tests, but I'd start from checking what we can keep in the error message. Because I don't think we can expect that data.originalMessage will be printed in any reasonable form. Unless... we'd have a special rule in CKEditorError that if data is an instanceof an error, it then gets printed nicely. Because, do we actually need to have data == { originalError: foo }? Can't we have data == foo?

[ma2ciek]

Because, do we actually need to have data == { originalError: foo }? Can't we have data == foo?

We can't have an error instance passed directly, because the JSON.stringify( error ) returns {}, thus I enumerated there the error properties:

{
    originalError: {
        message: err.message,
        stack: err.stack,
        name: err.name
    }
}

Of course, this data could be kept also as such structure:

{
    message: err.message,
    stack: err.stack,
    name: err.name
}

But it won't change anything in case of displaying the error stack properly.

[ma2ciek]

The other thing that comes to my mind is that we can overwrite the stack trace of the CKEditorError with the original error's stack trace as these errors start in the same place.

E.g.:

static rethrowUnexpectedError( err, context ) {
    if ( err.is && err.is( 'CKEditorError' ) ) {
        throw err;
    }

    const ckeditorError = new CKEditorError( 'unexpected-error: Unexpected error occurred.', context, {
        message: err.message,
        name: err.name
    } );

    ckeditorError.stack = err.stack;

    throw ckeditorError;
}

This construction introduces a little bit of magic but it should make the error stack look good and be clickable.

EDIT: Actually we could override the name and message (partially) too, so it will look like the normal error, but we'll gain an ability to restart the editor.

[ma2ciek]

cc @mlewand, what do you think about the proposed solutions?

[ma2ciek]

ping @Reinmar @mlewand, @pjasiun, could you check my proposals in the above comments?

[pjasiun]

I believe that we should forward errors when the editor is built with --debug flag, instead of wrapping them in CKEditorError. I hope it should be enough to have both easy debugging in the debug mode and working watchdog in the productive mode.

[ma2ciek]

I've checked and on Chrome replacing only the error.stack property results in displaying this stack to the console (and only this) when an error occurs:

[Reinmar]

If it works well in all browsers, then I think it's a good idea. If not, I'd do a custom error message getter for cases where error's additional data is an instance of an error.

[ma2ciek]

This replacement, unfortunately, doesn't print the stack trace well on Safari. It works well on Chrome and FF though. On Safari the stack trace is created once the error is thrown, not when the error is created, thus replacing it doesn't change anything.

Comparison of the original errors and the rethrown errors:

Chrome:

Firefox:

Safari:

The original error could be logged, however it doesn't sound like a good solution.

[jodator]

At least it's something better for majority of users :)

[Reinmar]

I guess we can change this multiple times – i.e. try with this and then improve if we'll see that it wasn't enough.

Optionally, if you think that the above result is still bad, we can add do it our // if CK_DEBUG comment, as @pjasiun proposed, to disable the rethrow completely in our entire dev env. This way, we'll have no problems during development at all (due to the flag) and the community and us in some specific cases (like debugging something in docs) will have pretty good errors thanks to the first step.

[ma2ciek]

I guess we can change this multiple times – i.e. try with this and then improve if we'll see that it wasn't enough.

Great 👍

Optionally, if you think that the above result is still bad, we can add do it our // if CK_DEBUG comment, as @pjasiun proposed, to disable the rethrow completely in our entire dev env. This way, we'll have no problems during development at all (due to the flag) and the community and us in some specific cases (like debugging something in docs) will have pretty good errors thanks to the first step.

Can we add the // @if CK_DEBUG // throw err; (where err is the original error) at the beginning of the CKEditorError.rethrowUnexpectedError? This way we should handle all possible debugging obstacles connected with rethrowing errors.

[Reinmar]

AFAIU rethrowing will still change the stack trace on Safari? That'd be a bummer, but I guess the only alternative would be something like:

        try {
            if ( this._pendingChanges.length === 0 ) {
                // If this is the outermost block, create a new batch and start `_runPendingChanges` execution flow.
                this._pendingChanges.push( { batch: new Batch(), callback } );

                return this._runPendingChanges()[ 0 ];
            } else {
                // If this is not the outermost block, just execute the callback.
                return callback( this._currentWriter );
            }
        } catch ( err ) {
                        // @if CK_DEBUG // throw err;
            CKEditorError.rethrowUnexpectedError( err, this );
        }

But I don't know how the above will differ from what you proposed. Could you show some stack traces that both solutions will produce?

[ma2ciek]

Oh. Now I reread this and this approach doesn't make any sense on Safari. The only way to preserve the original stack trace there would be commenting the try {, and } catch ( err ) { ... } parts. So actually the // @if CK_DEBUG // throw err; doesn't make sense to me.

The result of adding the // @if CK_DEBUG // throw err; part in the catch() is following:

And this is a problem for all rethrowing mechanisms on Safari, without logging the original error the stack trace is lost 😞

So maybe instead we could do // @if CK_DEBUG // console.error( err ); before rethrowing?

[Reinmar]

Dunno. Let's see it live and decide. We're not debugging things on Safari too often anyway, so we could most likely live without this anyway.

Do changes that you think will be fine, let's merge them and if we'll be still complaining then we can think further.