Ckeditor5: Write ESLint rule for CKEditorError's

[Reinmar]

📝 Provide a description of the improvement

• https://github.com/ckeditor/ckeditor5/issues/5643
• Missing @error.
  
  
  
  • This one is tricky as the error definition may be in a different file. We may need to have some /** @error other-file-path... */ to reference them.
  
  

---

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

[Reinmar]

I wanted to add this at the beginning of CKEditorError() but it fails in too many places (tests, most likely) for me to finish this:

        if ( !message.match( /[^:]+: [^.]+\./ ) ) {
            throw new Error( 'Wrong CKEditorError message format.' );
        }

[ma2ciek]

Optionally this could be a part of the documentation validator. But IMO the faster an error appears the better.

[ma2ciek]

We could also use https://eslint.org/docs/rules/valid-jsdoc for the default set of JSDoc tags.

[ma2ciek]

Oh, I saw it's deprecated.

[ma2ciek]

However, there's also a community plugin - https://github.com/gajus/eslint-plugin-jsdoc

[Reinmar]

I wonder if the part of the error after the colon is needed. I think I wanted it to be there so if you're having an error in the dev version of CKE5, you get the full title. But perhaps it's not needed anymore? Although, if an error is described only by its id and has no definition in  @error (because someone forgot to write it), then figuring out what it means may be tricky.

So, having @errors for all errors would be more important in this case.

[Reinmar]

In the past, I wrote that the message description in the error itself is necessary (https://github.com/ckeditor/ckeditor5/issues/5643#issuecomment-547411083) but now I think that it's overhead. I think that we could remove that bit from all errors and improve their error codes and error doc strings.

It'd be like:

/**
 * The {@link module:utils/collection~Collection#add `Collection#add()`} method was called without
 * an item. This method always needs to be executed with an item. And so on...
 *
 * @error collection-add-called-without-an-item
 */
throw new CKEditorError( 'collection-add-called-without-an-item', this );

Instead of:

/**
 * Missing item.
 *
 * @error collection-add-invalid-call
 */
throw new CKEditorError( 'collection-add-invalid-call: Missing item.', this );

The latter example looks like many errors that we have. The id (`collection-add-invalid-call`) does not help you understand what happened. The message that follows is often poor as well. And the documentation does not extend the error. 

We can remove the message if we'll focus on making the ids clearer. And the documentation should clearly explain what was wrong and how to do that right (if possible).

[Reinmar]

cc @AnnaTomanek @oleq :point_up:

[Reinmar]

cc @jodator :point_up:

[jodator]

Should be fairly ease - since I've implemented other custom rule I might check for this.

[jodator]

@Reinmar It would be good to agree on the requirements. Below is an example error in IDE using POC (ckeditor/ckeditor5-dev@3473815) of this rule:

[jodator]

To sum up what we need to have is:

1. Message format: 'error-id: Error message' vs 'error-id' 
2. Do we want validate other arguments as well (I assume that to much hassle).
3. What's the rule for detached @error definition with multiple throws (ie case for marker-destroyed). If it is in the context of the same file it might be easy to find defintion - however cumbersome. Having a rule that for every CKEditor we must have @error annotation in comment might be easier to implement.

[oleq]

We can remove the message if we'll focus on making the ids clearer. And the documentation should clearly explain what was wrong and how to do that right (if possible).

I second this: minimal code, verbose docs. I've been wondering why we duplicate these for a while but I couldn't figure it out.

[jodator]

I was debugging my inspection why it produces an error and just found out this:

It took me 5 debug sessions to spot why there's an error - it proves IMO that this check would be useful.

[jodator]

Example changes executed with a fixer: f729c2f

[jodator]

At this moment I can detect existing @error in the same file. However errors are defined in different files but also I've found many error mismatches (total 63 errors for both cases):

https://github.com/ckeditor/ckeditor5/blob/f729c2f7fdd07e6d9f039c5b5f40823eec4364b2/packages/ckeditor5-engine/src/model/treewalker.js#L59-L63

https://github.com/ckeditor/ckeditor5/blob/f729c2f7fdd07e6d9f039c5b5f40823eec4364b2/packages/ckeditor5-ui/src/view.js#L460-L470

[jodator]

And the only thing to cover is external errors (IE those thrown by editor implementation) like here: https://github.com/ckeditor/ckeditor5/blob/57188cb96671ea02939c3cf5e811e4ebebeb9fef/packages/ckeditor5-editor-decoupled/src/decouplededitor.js#L227-L231

What doesn't work is adding empty /** @error editor-wrong-element */ jsdoc - it will be detected as a duplicated error.

I think that we could mark it as "private @error " 

if ( isHTMLElement && sourceElementOrData.tagName === 'TEXTAREA' ) {
    // @error editor-wrong-element - documented in core/editor/editor.js
    throw new CKEditorError( 'editor-wrong-element', null );
}

Another format would work as well but I think that this would be easiest to parse.

[jodator]

Bonus question: what to do with errors that have one upper case like model-selection-setTo-not-selectable?

I'd go for enforcing lower case as this looks odd.

[jodator]

To sum up again:

• [x] Error id format error-fooBar-baz vs error-foobar-baz. (probably error message need to be refactored to errorId or name).
• ~external errors annotation (~~proposition~~)~ agreed that we can supress the error for now.
• [x] Optional - invalidate also empty jsdoc comment with @error only - also doable in minimal time.

I'd ditch params validation - to much work.

The change is read-ish to apply but would require adding missing annotations in the code base.