Problem/Motivation

CKEditor is using umberto for generating their documentation. It's an internal tool which they are using. It would be useful to improve our documentation by reference relevant parts of CKEditor code. Ideally, when code from CKEditor is being referenced in our code, it is something that IDEs are able to figure out, and it would be possible to navigate using their shortcuts to the relevant part of the CKEditor code.

Proposed resolution

  1. Create an intermediate type that maps to the CKEditor 5 module:
    /**
     * @typedef {module:engine/model/element} module:engine/model/element~Element
     */
    
  2. Provide CLI tool that automatically generates this mapping for all types that can be mapped

Steps for testing

  • In the IDE, associate the file type *.jsdoc as aJavaScript file. to take advantage of the mapping provided by the generated .jsdoc file.
  • core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/src/drupalmediaediting.js, and see if your IDE recognizes modelElement argument type. When the argument type is recognized, the IDE should be able to provide guidance on what are the accessible properties of that type.
  • If this does not work, it is possible your IDE is not including node_modules/@ckeditor5 is in its code completion index. For example, in PHPStorm, it may be necessary to explicitly mark the directory as "Not Excluded"

Issue fork drupal-3248423

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

Wim Leers’s picture

Title: Decide how CKEditor provided types should be referenced » Decide how CKEditor 5-provided types should be referenced
Component: Code » Documentation
Issue tags: +DX (Developer Experience), +JavaScript
Wim Leers’s picture

Project: CKEditor 5 » Drupal core
Version: 1.0.x-dev » 9.3.x-dev
Component: Documentation » ckeditor5.module

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Wim Leers’s picture

nod_’s picture

So I have a way to make documentation show up in PhpStorm but it's not pretty.

  1. First the ckeditor5 packages needs to be in node_modules (a yarn install is sufficient)
  2. In PhpStorm configure a new library in File | Settings | Languages & Frameworks | JavaScript | Libraries, add a new library, which points to the folder node_modules/@ckeditor
  3. Replace things like @param {module:engine/model/element~Element} modelElement by @param {module:engine/model/element} modelElement so that the default export is used for the content of the this.
  4. Once the above is done, there is code autocompletion as expected.

I don't think we can have a working out of the box experience for IDE autocompletion.

nod_’s picture

nod_’s picture

Spent some time with lauriii looking at the issue.

What works in PhpStorm:

  1. Associate the file type *.jsdoc as javascript files
  2. Create an intermediate type that maps to the Ckeditor module:
    /**
     * @typedef {module:engine/model/element} module:engine/model/element~Element
     */
    

    This takes advantage of the fact that referencing {module:engine/model/element} makes the autocompletion work while keeping the same declaration that ckeditor folks have in their code.

I'll work on automatically generating a mapping for ckeditor types so we can provide a ckeditor5.types.jsdoc file that will make IDE autocompletion work as intended.

nod_’s picture

Version: 9.4.x-dev » 10.0.x-dev

nod_’s picture

Status: Active » Needs review

yolo solution, need more docs to explain what's happening but should be somewhat helping.

nod_’s picture

tweaked the regex, only 86 files ignored from the ckeditor source. I think I caught all the export default, and @interface alias declarations in the code.

The files that don't match are files with several exports.

[10:04:07] CKEditor5 types have been generated: 490 declarations aliased, 86 files ignored
Done in 0.10s.
lauriii’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I tested some common classes that we use in our plugins such as writers, elements and there seems to be pretty good coverage. Before this patch, I'd say 0% of CKEditor types were recognized by PHPStorm. After this patch, maybe 70-80% are, which is already a huge DX improvement. Luckily the approach on this issue doesn't require anything special from when we document types in our code, meaning that all of the magic is behind the scenes and could be changed easily. This means that we could be easily iterate on this in future if we come up with new ideas what could improve this.

nod_’s picture

Also we can just get rid of it altogether when IDEs become "smart enough" :)

bnjmnm’s picture

Issue summary: View changes
lauriii’s picture

Issue summary: View changes
bnjmnm’s picture

Issue summary: View changes
bnjmnm’s picture

Issue summary: View changes

This is a clever solution. Cksource has a proprietary doc generation standard (Umberto) that IDEs such as PHPStorm do not currently support. The ckeditor5-types-
‎documentation.js‎ script scans the contents of node_modules/@ckeditor5 for Umberto definitions. These definitions are used to dynamically generate a file - ckeditor5.types.jsdoc - that uses JSDoc to map IDE-understood typedefs to their actual definitions in CKEditor5. ckeditor5.types.jsdoc needs to be recognized as a JavaScript file in order to work so the IDE knows to parse the mapping as JSDoc.

This is a big DX improvement. It has zero impact on any runtime code so introducing these improvements now introduces no risk or debt. It can be improved on later if we wish, or we can just enjoy the considerable improvements introduced here.

Will commit once tests complete after the rebase I just performed.

lauriii’s picture

I guess something worth noting is that the syntax CKEditor is using is standardized in JSDoc 3 (ES 2015 modules and namepaths). It just isn't supported by most of the tooling yet (including IDEs). However, CKSources proprietary doc generation tool does support it, and hopefully other tools will also provide support in the future, which will allow us to remove the mapping. The workaround that we apply is that we remove the inner member in the mapping from modules that don't need it because they only have one inner member. Because of that, this solution cannot handle 100% of CKEditor code base.

bnjmnm’s picture

Although it's likely quite safe for a number of reasons, I'm reluctant to Gitlab-merge an MR that needs rebasing until after the tests have fully run post-rebase.

Unfortunately, I keep missing the window between tests passing and not needing another rebase (the window often closes while tests run), so here's a patch of the current MR that I'll be using to commit this to D10.

  • bnjmnm committed aaed740 on 10.0.x
    Issue #3248423 by nod_, lauriii: Decide how CKEditor 5-provided types...
bnjmnm’s picture

Version: 10.0.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 10.0.x, very nice addition.

Will see if 9.4.x is cherry pickable.

  • bnjmnm committed eecd128 on 9.4.x
    Issue #3248423 by nod_, lauriii: Decide how CKEditor 5-provided types...
bnjmnm’s picture

Cherry picked to 9.4.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

catch’s picture

Version: 9.4.x-dev » 9.3.x-dev
Status: Closed (fixed) » Needs work

Re-opening for 9.3.x backport.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
97.55 KB

Patch from local chery-pick.

  • lauriii committed 5e7f9e0 on 9.3.x
    Issue #3248423 by nod_, Wim Leers, lauriii: Decide how CKEditor 5-...
lauriii’s picture

Status: Needs review » Fixed

Committed 5e7f9e0 and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.