[DX] Limit the use of 'version' in hook_library_info() specifically to things that are 3rd party libraries

[klonos]

I was working on #6555 and was trying to figure out why changes I was adding to the ckeditor5.admin.js file would not take. Clearing caches would not solve it, so it left me head-scratching and wondering what I might be doing wrong. There was quite a waste of time before I finally discovered what was happening...

Updating other .js files in other parts of the admin UI where other modules are adding summary text to vertical tabs was working as expected, and changes would take straight away or after a simple cache clear. So this was clearly happening with ckeditor-related .js files. Inspecting the various .js assets loaded, I noticed that those that worked as expected had their query string set to a random token (coming from css_js_query_string - see _backdrop_flush_css_js()). The various ckeditor5-related .js assets on the other hand had v=1.29.x-dev as their query string. That was effectively causing the files to be cached "persistently", and that explained why my changes wouldn't take ...despite clearing caches etc. I then tried Re-enabling JS aggregation, and that would indeed resolve the problem (because these files were not loaded as stand-alone assets any longer, so they didn't have the "fixed", version-based query string that caused the persistent caching).

I've tested locally, and there are a couple of ways to work around this:

• changing the logic in backdrop_pre_render_scripts() so that a random token is also added to the version-based query string of .js assets
• removing the 'version' => $info['version'] from custom (Backdrop-specific, non-3rd-party) .js in ckeditor5_library_info()

Noting that this problem doesn't apply to .css assets, as there is no logic currently in backdrop_pre_render_styles() to be adding the 'version' string as a query string.

So the above, along with time wasted trying to figure out things has got me thinking: who does this problem affect...

• Non-developer, end users (people that will never need to touch the .js code in their Backdrop sites): this group of people will not be affected.
  • They have JS aggregation enabled. So all good 👍🏼
  • When they update their site, the version string that is added as a query string to .js assets loaded gets updated to reflect the new version of Backdrop. So any persistently-cached .js file is cache-busted. This is the intended functionality. So all good 👍🏼
• Developers and especially core contributors: Chances are that these people disable their JS aggregation on their locals while working with code. That causes this problem here and, unless they know what's causing it and how to work around it, it will cause them grief/frustration and waste of time. DX 👎🏼

It also got me thinking: I know why we are adding the version string to the asset query (more effective/efficient caching), but when should it be added? As it is currently, if you omit it from your hook_library_info() implementation, you get Warning: Undefined array key "version" in backdrop_get_library() PHP errors. That means that developers will always try to add it. And when they wonder "what version should I use for this string?" ...the obvious thought would be either BACKDROP_VERSION, or in the case of modules like CKEditor 5, the same version as the 3rd-party library. This is wrong though: "fixing" the version of .js assets to the version of core when developing locally will cause the query string to always be something like ?v=1.29.x-dev, and that will cause them the issue described here. It does make sense to be adding the version string to the actual 3rd party libraries (files like core/modules/ckeditor5/lib/ckeditor5/build/ckeditor5-dll.js or core/misc/smartmenus/jquery.smartmenus.js or the various core/misc/jquery*.js files etc.), but we should be discouraging people from adding it to the custom .js that we use in the admin UI, like core/modules/ckeditor5/js/ckeditor5.admin.js. Right?

If we want to be adding the same version sting to those custom .js assets as the 3rd party library they are related to (as a way of "bundling them"), then can we at least offer another Do not add the version to the query string of JS assets option in admin/config/development/performance? That way, we allow developers/contributors to enable that option in their locals, and they won't be wasting the time I wasted trying to figure out why their code doesn't work.

Another thought I had: are the .js and .css files added by ckeditor5_library_info() implementation an oversight? Were they left there after moving the module from contrib to core perhaps? Should they instead be loaded via #attached or backdrop_add_js() or some other way as needed? ...or is this a more generic problem? ...in which case we should also update the docblock of hook_library_info() to warn people of the potential caching issues that might be caused by "fixing" 'version' to a specific string?

Thoughts?

[klonos]

So, some suggestions that we should consider (we can do all, or some of these):

• make the 'version' in hook_library_info() optional
• add some help text in the docblock of hook_library_info() to explain that 'version' plays a part in .js asset caching, and noting that that might cause potentially undesired "persistent caching"
• add a new setting in admin/config/development/performance that allows skipping the addition of the version string to .js assets when working with code (use the same random string as .css assets instead)
• remove 'version' from .js/.css assets defined in ckeditor5_library_info() if these assets are not coming from CKEditor (the 3rd party library) ...also check if we are doing that elsewhere
• remove .js/.css assets defined in ckeditor5_library_info() and load them via #attached or backdrop_add_js() as needed instead.

[indigoxela]

As a maintainer of several modules using hook_library_info (TinyMCE, FullCalendar, Leaflet...), I was aware of that version string query part and actually always found it beneficial. Never ran into trouble with it.

That the custom admin JS (settings form) is also defined as library, is odd, though. I can see, how this leads to unexpected behavior, when working on the admin UI.

Limiting to only 3rd party code won't be possible, BTW, as there's always the "integration JS", that's part of the defined library, but not part of the 3rd party code.

[klonos]

Limiting to only 3rd party code won't be possible, BTW ...

Yeah, I know that it is very difficult to distinguish between what can be 3rd party code vs. what not in a technical way that allows to put some logic to it. I was thinking these tasks at least:

1. Update the docblock available for hook_library_info(), to "softly discourage" people, mention the intended usage for 'version' (3rd party scripts), warn them about these odd issues, or at least let them know that this plays a part in cache clear.
2. Make sure that we do not consider 'version' required, so that people don't feel the need to add "some" version all the time. As it is now, omitting it throws warnings. Perhaps setting a default of blank/NULL might help. Specifically in the logic in backdrop_pre_render_scripts() there's a one-line conditional that we could add, which makes all these PHP warnings go away - there is no other place in core that makes 'version' "required" actually.

...there's always the "integration JS", that's part of the defined library, but not part of the 3rd party code.

Can you please elaborate a bit further on that? I assumed that that was the case with some of the libraries defined in ckeditor5_library_info(), but unsure which ones qualify as the "integration" bit. I'm trying to understand better. Is it the set of /js/ckeditor5.js and /js/ckeditor5.formatter.js? Perhaps also all the additional .js for the various plugins? ...I was also thinking the follwing perhaps:

• Move the code in /js/ckeditor5.admin.js to another file, like ckeditor5.toolbar.js or ckeditor5.filters.js or something.
• Reserve the ckeditor5.admin.js file for anything that has to do with generic, non-CKE5-specific things in the admin UI (like the summary text in the vertical tabs). I believe that the *.admin.js files are (even "unofficially", as a best practice) reserved for such things anyway. Right?

I'm still considering various options on how to solve this best. I'll file a couple of PRs with some things that worked for me that might be able to be done independently from any decision we make here.

[indigoxela]

...there's always the "integration JS", that's part of the defined library, but not part of the 3rd party code.

Can you please elaborate a bit further on that?

I'm usually not good at explaining. :wink:

In none of modules I maintain, I've ever used hook_library_info() for anything other than 3rd party libs plus integration code. But maybe that's just me. :shrug:

For CKE5 actually everything except ckeditor5.admin.js is integration stuff. That admin script with its dependencies and CSS is only used to assemble the editor toolbar, but isn't in any way involved with the actual editor.

That backdrop.ckeditor5.admin has probably been defined as library to easily add dependencies on the sortable/droppable UI libs, and the CSS involved. But I'm only guessing here. Note that backdrop.ckeditor5.admin isn't supposed to be re-used by other modules.

It would be possible to add these admin UI related files simply as individual components, instead of a defined library, with the "#attached" in ckeditor5_settings_form(). Not sure, how much this helps to prevent WTF when working on the admin UI.

[klonos]

For CKE5 actually everything except ckeditor5.admin.js is integration stuff.

That's what I thought. Thanks for clarifying that 👍🏼

Note that backdrop.ckeditor5.admin isn't supposed to be re-used by other modules.

Yup, I got that 👍🏼 ...it's just that I was expecting to have something like ckeditor5.js (like we have core/modules/node/js/node.js and core/modules/node/js/node.types.js and core/modules/path/js/path.js and core/modules/path/js/path.admin.js etc.), but because that is reserved for the actual 3rd-party library itself, I would then expect a ckeditor5.admin.js to hold non-CKE5-specific admin-related JS. But that is also taken by something else as well. I could name the .js file that handles the summary text something else entirely, but the *.admin.js is something that I would expect as a developer/contributor. That's why I am suggesting that we rename backdrop.ckeditor5.admin to backdrop.ckeditor5.toolbar (because if you look at what the code in the .js assets that library loads is doing, you'll see that it's 99% related to CKE5 toolbar and buttons configuration). ...and yes, I would then load the new ckeditor5.adin.js file using #attached or similar, and only specifically in the admin/config/content/formats/* forms that use CKE5.

Thanks once again @indigoxela 🙏🏼

[klonos]

...or do you think better add the JS for the vtab summaries in admin/config/content/formats/* forms inside core/modules/filter/js/filter.admin.js instead? 🤔

[indigoxela]

...or do you think better add the JS for the vtab summaries in admin/config/content/formats/* forms inside core/modules/filter/js/filter.admin.js instead?

I don't think that works, no. There are other (contrib) modules providing other editors, there may not even be an editor attached at all. It's not the Filter module's job to set CKEditor5 summaries. :wink:

[klonos]

Also thought so 😅

[klonos]

Here's a first PR: https://github.com/backdrop/backdrop/pull/4760

• makes sure that 'version' is treated as optional in backdrop_get_library() - this is the main thing actually: backdrop/backdrop@1c2a070 (#4760)
• adds explanation for 'version' in the docblock for hook_library_info() (+ some additional @sees)
• switches the 'version' for the libraries in ckeditor5_library_info() to be using the CKEDITOR5_VERSION constant instead of pulling in the version of the ckeditor5 module (which will always be the core version now that the module has moved into core). This should make it easier when updating the CKE5 library to newer versions on locals with JS aggregation disabled.

[klonos]

...btw, not sure why we are not adding the version string as a query when loading .css assets and we are only doing that for .js assets. If the CSS belongs to the 3rd party library, then that should also be treated the same (separate issue I think).

[klonos]

...another PR to consider: https://github.com/backdrop/backdrop/pull/4761

• adds a new option in admin/config/development/performance, which is hidden via #states and only becomes available when JS aggregation is disabled (so it effectively becomes a developer-only setting, which most production sites will never see).
• the option forces the query for the .js assets to always use the random dummy string set in the css_js_query_string state, same as all other assets, regardless of whether 'version' has been specified or not.

[klonos]

I just got bitten again by this 😞 ...I was working on #6591 and as part of that I was trying to edit core/modules/ckeditor5/js/ckeditor5.js. I kept adding various console.log() in order to debug, or making other code changes, but nothing was happening. To work around those issues temporarily, I changed the following in backdrop_pre_render_scripts():

-  $query_string = empty($item['version']) ? $default_query_string : 'v=' . $item['version'];
+  $query_string = $default_query_string;

I'll loop back to this.