Enable aggregation for CKEditor 5 assets

Rerolled the patch in #2, thanks!

Manual testing: 🥳

Before

<script src="/sites/default/files/js/js_3f6AdoXJIzKJzozcBSEx3O4Lsr0LtR5J6PDqCGp8MzQ.js"></script>
<script src="/core/assets/vendor/ckeditor5/ckeditor5-dll/ckeditor5-dll.js?v=31.1.0"></script>
<script src="/core/assets/vendor/ckeditor5/language/language.js?v=31.1.0"></script>
<script src="/core/assets/vendor/ckeditor5/editor-classic/editor-classic.js?v=31.1.0"></script>
<script src="/core/assets/vendor/ckeditor5/html-support/html-support.js?v=31.1.0"></script>
<script src="/core/assets/vendor/ckeditor5/editor-decoupled/editor-decoupled.js?v=31.1.0"></script>
<script src="/sites/default/files/js/js_WnGf9r4hVWXVSakVLCqYSNeD-o5zMho9-7cD_MuO_10.js"></script>
<script src="/core/assets/vendor/ckeditor5/basic-styles/basic-styles.js?v=31.1.0"></script>
<script src="/core/modules/ckeditor5/js/build/drupalEmphasis.js?v=9.4.0-dev"></script>
<script src="/core/assets/vendor/ckeditor5/image/image.js?v=31.1.0"></script>
<script src="/core/modules/ckeditor5/js/build/drupalImage.js?r7pst1"></script>
<script src="/core/assets/vendor/ckeditor5/heading/heading.js?v=31.1.0"></script>
<script src="/core/assets/vendor/ckeditor5/essentials/essentials.js?v=31.1.0"></script>
<script src="/core/modules/ckeditor5/js/build/drupalHtmlEngine.js?r7pst1"></script>
<script src="/sites/default/files/js/js_C9cya7gpmllS5d8JGlnvW3QuoAq0nRi4yIFgawfTytA.js"></script>
<script src="/core/modules/ckeditor5/js/build/drupalMedia.js?r7pst1"></script>
<script src="/core/assets/vendor/ckeditor5/block-quote/block-quote.js?v=31.1.0"></script>
<script src="/core/assets/vendor/ckeditor5/link/link.js?v=31.1.0"></script>
<script src="/core/assets/vendor/ckeditor5/list/list.js?v=31.1.0"></script>
<script src="/core/assets/vendor/ckeditor5/paste-from-office/paste-from-office.js?v=31.1.0"></script>
<script src="/core/assets/vendor/ckeditor5/source-editing/source-editing.js?v=31.1.0"></script>
<script src="/sites/default/files/js/js_Y0zfNrRXQ4omiZN6AW4VL6YkoEGl_tMFkTAGQmS30mw.js"></script>
<script src="/core/assets/vendor/ckeditor/ckeditor.js?v=4.17.1"></script>
<script src="/sites/default/files/js/js_GRmKOoYReYJJptbNu9iGok1QTSnvcAryp8m_mvSx9IY.js"></script>

After

<script src="/sites/default/files/js/js_nQtm9zpwk-PrJ2wl0Z0AldpA96w-3UP-ezA8D4cnBqY.js"></script>
<script src="/core/modules/ckeditor5/js/build/drupalHtmlEngine.js?r7pu9q"></script>
<script src="/sites/default/files/js/js_MINxMjLxQhsjQve9n-dob_p2zWdwgmqtCm5-9iUtjr8.js"></script>
<script src="/core/assets/vendor/ckeditor/ckeditor.js?v=4.17.1"></script>
<script src="/sites/default/files/js/js_GRmKOoYReYJJptbNu9iGok1QTSnvcAryp8m_mvSx9IY.js"></script>

Code review

1. +++ b/core/modules/ckeditor5/ckeditor5.libraries.yml
   @@ -38,21 +38,23 @@ drupal.ckeditor5.quickedit-temporary-work-around:
   +    - core/ckeditor5
   ...
   +    - core/ckeditor5
   

   Are these changes truly necessary? They're implied by their other dependencies?

2. Needs reroll now that #3228464: API for contrib projects to load CKEditor translations landed.

I left ckeditor-dll.js file unaggregated so that all the ckeditor 5 plugins and translations end up in a separate aggregate from the rest of the JS on the page.

kept the added dependencies to core/ckeditor5 since the plugins need the main library to do anything, they do have a dependency on it. Implicit dependencies are bad #3244855: Regression in Drupal 9.3.x due to jQuery.once deprecation :)

#5: so … diff-wise … you only changed the very first hunk in the file?

That is correct

1. +++ b/core/core.libraries.yml
   @@ -30,6 +30,9 @@ ckeditor5:
   +    # This file is not aggregated so that ckeditor5 assets can be grouped
   

   Übernit: s/ckeditor5/CKEditor 5/

2. +++ b/core/core.libraries.yml
   @@ -30,6 +30,9 @@ ckeditor5:
   +    # @see ckeditor5_js_alter().
   

   Übernit: the trailing period here should be removed.

IMHO #5 is fine, and we can take it further in #3224261: [PP-2] Front-end performance: concatenate CKEditor 5 JS assets for every unique configuration.

… by which I meant to do this 🤓

Isn't it just that everything depending on the ckeditor5 library has to be loaded after this file, and because it's not aggregated, that means they get their own aggregate?

That is correct, in core currently it means that everything between ckeditor5-dll.js and ckeditor.js (the version4) gets it's own aggregate, when ckeditor4 is removed it's an aggregate for everything after ckeditor5-dll.js (so that includes some toolbar, bigpipe and a few others). Still better than nothing.

Fixed the comment, ideally it's a bit more accurate.

Ideally if we could ensure that all the ckeditor 5 libraries are added at the end of the #attached array before it's being processed we could make sure that the aggregation group contains only cke5 files and would make the other aggregate probably more useful. But we don't have a way to alter the position of a library in the #attached array, and i'm not about to add a weight property to the library definition (that would work though), i'd prefer #3232810: [PP-1] Allow setting aggregation groups for js files in library definitions.

Patch review

1. +++ b/core/core.libraries.yml
   @@ -30,6 +30,11 @@ ckeditor5:
   +    # translations, enabled plugins, and the rest of the javascript needed on
   

   s/javascript/JavaScript/

2. +++ b/core/core.libraries.yml
   @@ -30,6 +30,11 @@ ckeditor5:
   +    # the page
   

   Missing railing period.

3. … but not marking Needs work because @catch RTBC'd it already :D Should be easy to fix on commit and cherry-pick. Queued tests for 9.3 and 9.4, hopefully those pass too. If the patch needs to be rerolled, then we should fix the two nits above.

Manual test

#4

<script src="/sites/default/files/js/js_nQtm9zpwk-PrJ2wl0Z0AldpA96w-3UP-ezA8D4cnBqY.js"></script>
<script src="/core/modules/ckeditor5/js/build/drupalHtmlEngine.js?r7pu9q"></script>
<script src="/sites/default/files/js/js_MINxMjLxQhsjQve9n-dob_p2zWdwgmqtCm5-9iUtjr8.js"></script>
<script src="/core/assets/vendor/ckeditor/ckeditor.js?v=4.17.1"></script>
<script src="/sites/default/files/js/js_GRmKOoYReYJJptbNu9iGok1QTSnvcAryp8m_mvSx9IY.js"></script>

#12

<script src="/sites/default/files/js/js_4mga7FT7b_S-sIraMGtUX5CmiheSkXq1Wy_sTbCnQ7g.js"></script>
<script src="/core/assets/vendor/ckeditor5/ckeditor5-dll/ckeditor5-dll.js?v=31.1.0"></script>
<script src="/sites/default/files/js/js_l3tFjJj9_-_TjRM-Y6wOtSedwELEwZnli5pBYj4zFTU.js"></script>
<script src="/core/assets/vendor/ckeditor/ckeditor.js?v=4.17.1"></script>
<script src="/sites/default/files/js/js_GRmKOoYReYJJptbNu9iGok1QTSnvcAryp8m_mvSx9IY.js"></script>

Looking good, and #12 is indeed better than #4. 👍