Introduce error logging mechanism

[Piotrek Koszuliński]

The problem

Currently, if we want to log some error or warning we do this:

// jshint ignore:start
else { // %REMOVE_LINE%
	window.console && console.log( '[CKEDITOR.dom.selection.reset] Wrong selection instance resets fake selection.' ); // %REMOVE_LINE%
} // %REMOVE_LINE%
// jshint ignore:end

Or sometimes this:

if ( !editor.config.mathJaxLib && ( window.console && window.console.log ) ) {
	window.console.log( 'Error: config.mathJaxLib property is not set. For more information visit: ', docsUrl );
}

Or even this (async errors to avoid breaking the app by non-critical problems):

CKEDITOR.tools.setTimeout( function( name, pluginName ) {
	throw new Error( 'Plugin "' + name.replace( ',', '' ) + '" cannot be removed from the plugins list, because it\'s required by "' + pluginName + '" plugin.' );
}, 0, null, [ name, pluginName ] );

There are couple of issues with code like this:

• It's ugly and long. Messages (if not removed during building) are kept in the production ready code. So to avoid bloating it, we log very infrequently. This may need to change due to #11502.
• We do not have a fast option to turn this into code that throws to improve testability. The special case here and especially problematic is asynchronous code in which even if we throw an error, there's no way to catch it (OK, there's window.onerror, but that doesn't mute it AFAIK).
• There's no way to mute all console logs and errors if application requires it (see ​http://dev.ckeditor.com/ticket/11502#comment:16). There's no way to pipe them to some other output (e.g. a mechanism which sends them to the server).

Solution

I would like to propose introducing two new methods:

CKEDITOR.error( errorCode [, additionalData... ] );
CKEDITOR.warn( errorCode [, additionalData... ] );

And complementary boolean flag and an event:

CKEDITOR.verbosity = CKEDITOR.VERBOSITY_(ERROR/LOG/NONE) {Number}
CKEDITOR#log

What is errorCode? It's not any more a full message. In order to not keep the full strings inside our code base, we will export them to a guide in docs. Error could would be built from location of a source file and some id:

• 'range-startcontainer'
• 'mathjax-lib'
• 'editor-removeplugins'
• 'selection-fake-reset'

So a class/plugin/module + a message identifier. Then, CKEDITOR.error/warn will log also a link to a guide where all errors are explained. A bit less user friendly, but especially with growing number of messages it will allow us to drop few KB and also improve the messages as they will be able to be longer.

I know that it's controversial because error codes are less useful, but I don't see a much choice for CKEditor 4. To keep long error messages in the code base, so in development version of the editor errors are fully meaningful we would need to:

• Agree that we bloat the code with them and keep them also in the production code.
• Implement a new feature in the builder which would look for CKEDITOR.warn/error and replace them with shorter calls (hashes of the messages?) as well as produce a documentation for these hashes. Because we would need a guide for them anyway.

The second solution sounds great for CKEditor 5, but not for CKEditor 4. Therefore I proposed this a little bit controversial change.

Behaviour:

1. CKEDITOR.error()
   • If CKEDITOR.verbosity == CKEDITOR.VERBOSITY_ERROR, throws a real error with all the details.
   • If not, then it fires CKEDITOR#log with data.type = error and all the details.
2. CKEDITOR.warn()
   • Fires CKEDITOR#log with data.type = warn and all the details.
3. CKEDITOR#log has a default (low prior) listener which:
   • If CKEDITOR.verbosity == CKEDITOR.VERBOSITY_NONE, then do nothing.
   • If type == error, then tries to use console.error() (downgrades to console.log() if the former does not exist)
   • If type == warn tries to use console.warn() (downgrades to console.log() if the former does not exist).
   • In both cases, it logs all the details plus a link to documentation (error codes).

[Olek Nowodziński]

cc

[Frederico Caldeira Knabben]

As long as the code to implement this API will be smaller than the plain messages in the old way, than I'm fine with that :) (In other words, be sure to keep this super dumb and simple)

[Piotrek Koszuliński]

I'm afraid that the code (implementation of warn(), error() and #log things) will exceed size of the current messages (although, we'll also save space for window.console && window.console.log() checks), but that's not the point. The difference will not be big and eventually, after we'll add more warnings in the code, then they will indeed reduce the size comparing to the size without them.

But the point of having them is different – how can we log and throw in a controllable way (for us and other developers)?

[Frederico Caldeira Knabben]

I'm fine, ofc... my point was just about not over-engineering this, because this is merely a developer tool that will end up into distros.

[Marek Lewandowski]

I was worried that error codes would be a meaningless integer, here I see strings instead so that's better. Do you still think it's worth to use error codes instead of real, short messages?

editor-elementType vs editor: Expected CKEDITOR.dom.element instance

editor-elementMode vs editor: Unspecified element mode

editor-inlineIE vs editor: Inline mode is not supported on IE quirks

editor-invalidElement vs editor: Mode doesn't support "${element.getName()}" element

editor-loadPlugins-<something> vs editor.loadPlugins: Plugin ${name.replace( ',', '' )} cannot be removed, it's a ${pluginName} plugin dependency.

My point is that error codes simply brings extra complexity both for end-user and developer

• user has to use external resource to identify what CKEditor is trying to tell him
• developer needs to interfere with another project (sdk) in order to add the message

At the end, the idea of telling user what actually went wrong is great!

[Dusty Jewett]

As a user, I think this is great!

There is already a devtools plugin, why not have devtools override CKEDITOR.error() and CKEDITOR.warning() to look up the 'long' text and log that? Best of both worlds... The text is there, in the repo, and visible to developers who want it. But it doesn't have to be in the main js file, so it doesn't bloat the source.

Allowing/ensuring that CKEDITOR.error() and CKEDITOR.warn() are overridable also allows users who want to pipe exceptions into their current reporting pipeline (we use Ember and Opbeat, but angular, newrelic and airbrake are other examples).

If you add a lookup to devtools, current plan will provide:

1. Good defaults
2. Discoverability for new users
3. Configurability for regular users
4. Overridability for advanced users

As the commenter cited on the ticket, my main concern is the addition of CKEDITOR.warn() and CKEDITOR.error()... If it were in my work queue, I'd separate the concerns into two tickets... One to enable global handling of exceptions, and another aimed at balancing exception message size with developer discoverability.

[Szymon Kupś]

Pushed ​branch:t/13632.

One change that was discussed with Reinmar is that CKEDITOR.error() fires log event even when CKEDITOR.VERBOSITY_ERROR is used. This way, default log event listener will be always able to display additionalData and documentation URL. It will also allow custom listeners to report problems even if errors were thrown.

[Piotrek Koszuliński]

Just dropping it here – ​https://docs.angularjs.org/error. I'll comment on this later.

[Piotrek Koszuliński]

Do you still think it's worth to use error codes instead of real, short messages?

Yes. It won't necessarily be so important in CKEditor 4 where we do not perform that much validation, so there are few messages in the code and they can be rather short while still being pretty clear. But with complexity of CKEditor 5's data model we will need a better mechanism. First of all, there will be dozens of errors, second of all, they will be many times very complicated. This requires additional explanation, just as you can see in ​Angular.

So for CKEditor 4 I'm for the easiest possible solution right now – using short error codes and a manually maintained, external reference. That's the simplest thing to implement and since CKEditor 4 does not log many messages flaws of this solution can be ignored.

For CKEditor 5 we can go in two (not disjoint) ways:

1. As proposed by @dustyj-pn – we may have a special plugin which will extend the error codes with proper messages. As a matter of fact, most likely we'll need such a plugin anyway, because we will need additional tools for debugging data model.
2. But something still have to maintain the list of errors. The plugin does not solve all cases, because from time to time one need to debug something on production (or understand what a thrown error means). So a guide will need to exist. I also guess that we would like to keep details about errors close to the code which throws them, so this means that we will need to process the code to extract those messages from it, remove the details while building and put them into the guide and the dev tools plugin.

[Tade0]

For reference, here's how a typical error message looks in angular:

Error: [$injector:unpr] Unknown provider: $resourceaProvider <- $resourcea <- Products <- tpGridDirective
http://errors.angularjs.org/1.3.15/$injector/unpr?p0=%24resourceaProvider%20%3C-%20%24resourcea%20%3C-%20Products%20%3C-%20tpGridDirective
    at REGEX_STRING_REGEXP (angular.js:63)
    at angular.js:4015
    at Object.getService [as get] (angular.js:4162)
    at angular.js:4020
    at getService (angular.js:4162)
    at Object.invoke (angular.js:4194)
    at Object.enforcedReturnValue [as $get] (angular.js:4056)
    at Object.invoke (angular.js:4203)
    at angular.js:4021
    at getService (angular.js:4162)

And the minified version:

Error: [$injector:unpr] http://errors.angularjs.org/1.3.16/$injector/unpr?p0=%24resourceaProvider%20%3C-%20%24resourcea%20%3C-%20Products%20%3C-%20tpGridDirective
    at Error (native)
    at http://localhost:8000/lib/angular.js:6:417
    at http://localhost:8000/lib/angular.js:38:7
    at Object.d [as get] (http://localhost:8000/lib/angular.js:36:13)
    at http://localhost:8000/lib/angular.js:38:81
    at d (http://localhost:8000/lib/angular.js:36:13)
    at Object.e [as invoke] (http://localhost:8000/lib/angular.js:36:283)
    at Object.$get (http://localhost:8000/lib/angular.js:34:268)
    at Object.e [as invoke] (http://localhost:8000/lib/angular.js:36:315)
    at http://localhost:8000/lib/angular.js:38:110

Sometimes the error message can be pretty long:

Uncaught Error: [$injector:modulerr] Failed to instantiate module xxx due to:
Error: [$injector:nomod] Module 'xxx' is not available! You either misspelled the module name or forgot to load it. If registering a module ensure that you specify the dependencies as the second argument.

Notice, how the minified version does not have a verbose explanation - this string is left out during minification. A seasoned Angular developer usually doesn't need it anyway because all the necessary information is there.

The structure is as follows:

Uncaught Error: [$module:errshortname] <ErrorDescription> <URLtoDocs> <StackTrace>

Legend:

$module - name of the core module that encountered the error(most of the time it's the $injector, which manages dependency injection).

errshortname - id of the error. Usually an acronym of the error message.

<ErrorDescription> - A human-readable error message. Sometimes contains a hint as to what could be wrong. Not present in production.

If possible to obtain, contains a path of dependencies that lead to the problem, e.g:

$resourceaProvider <- $resourcea <- Products <- tpGridDirective

In this example 'tpGridDirective' directive tried to use the 'Products' service which was dependent on a '$resourcea' service, which in turn was not found by the $injector(no $resourceaProvider provider present).

<URLtoDocs> - URL to docs. Usually pretty general, but if there's a core module in the path it may lead to a more specific explanation.

<StackTrace> - Stack trace. Present in production, but not really informative there. In general it's useful only for Angular core developers.

[Piotrek Koszuliński]

While thinking about the next steps we realised one thing – changing verbosity from "throw" to "log" doesn't make sense as it will change the behaviour significantly. Even backing to some early returns won't help as throwing affects also the outer function calls.

We can define 3 types of problems:

1. Problems which should immediately kill the application. Such as incorrect CKEDITOR.replace() argument or inability to load some core class or plugin. Those need to throw errors regardless of any setting as they basically mean that the whole application is useless. Of course, these errors should be catchable, but that's outside of CKEditor 4's scope. Note also that we can throw a subclass of the Error type to implement features as Angular has (see comment:13).
2. The second kind of problems are those which will definitely affect the working of application, but which do not break it. For instance – undefined MathJax path, undefined file upload path, etc. Those issues should log errors, but they should not throw errors. For those we will introduce CKEDITOR.error() which should use console.error().
3. And the last kind of problems are those which may affect the working of application or which means that something went a bit wrong. Those need CKEDITOR.warn() which should use console.warn().

So we need to alter our initial plan – CKEDITOR.error() should never throw any errors.

Finally, we need a different verbosity setting. In this case the most useful solution will be a binary flag with two values defined:

• CKEDITOR.VERBOSITY_WARN (log all warnings)
• CKEDITOR.VERBOSITY_ERROR (log all errors)

the default value will be: CKEDITOR.VERBOSITY_WARN + CKEDITOR.VERBOSITY_ERROR. The logging mechanism can be muted by setting verbosity to 0.

[Piotr Jasiun]

cc

[Piotrek Koszuliński]

We need to speed up 4.5.3 so we won't be able to include this feature in it.

[Szymon Kupś]

Pushed ​branch:t/13632 with the new implementation.

[Szymon Kupś]

While this ticket is waiting for review I've pushed ​branch:t/13632-use with usage of CKEDITOR.warn and CKEDITOR.error methods in situations I've found suitable.
I've also created a draft of the error codes reference in guides: ​https://github.com/ckeditor/ckeditor-docs/tree/t/13632-use
It's format and location in guides needs some discussion.

[Piotrek Koszuliński]

Merged the t/13632 branch to master with ​git:2e699f0.

I extracted comment:19 to #13730.