Remove deprecated jQuery UI components and fork remaining source code into core

Assuming #3064049: Replace jQuery UI sortable with Sortable js and #3072906: Deprecate and remove jQuery UI datepicker land, the jQuery UI asset libraries still needed in D9 would be:

core/jquery.ui
core/jquery.ui.autocomplete
core/jquery.ui.draggable
core/jquery.ui.dialog
core/jquery.ui.menu
core/jquery.ui.position see: #3078116: Deprecate jquery.ui.position
core/jquery.ui.resizable
core/jquery.ui.widget

The forked source code can be limited to this list.

The core/jquery.ui includes many files used by the other asset libraries, some of these are no longer needed.

• escape-selector-min.js
• form-min.js
• form-reset-mixin-min.js
• labels-min.js

The core asset library can be trimmed down to just files that the remaining asset libraries require.

Edit: form-reset-mixin-min.js can be removed but it's not in core/jquery.ui - I'll check the folder for other files that are only used by deprecated asset libraries.
Edit: added jquery.ui.menu which was not deprecated and is required by autocomplete

I began working on this by removing everything jquery.ui in core.libraries.yml that was marked deprecated, including datepicker, sortable, touch-punch, checkboxradio and controlgroup.
However since those are (currently) still dependencies in core I kept them in this patch.

I also removed the dependency property from checkboxradio and controlgroup because they are specific dependents of button- which is a dependent of dialog.
In #4 I listed form-reset-mixin-min.js as a file which could be removed but it is a dependency of checkboxradio.

The big change in this patch is adding jQuery UI source files into the core/assets/vendor/jquery.ui folder in core.
These files come from the latest version, 12.1.1
https://github.com/jquery/jquery-ui/tree/1.12.1

I did some research on JavaScript minification, there are some open core issues:
#1490312: [META] Improving CSS and JS preprocessing
#119441: Compress JS aggregation
#1537198: Add a Production/Development Toggle To Core
#2258313: Add license information to aggregated assets

This requirement seems different for a few reasons:

1. Core already used minifed files for jQuery UI, no precedent for switching between dev and prod
2. The build script only needs to be run IF there are changes to source, not regularly like for the es6 files in core.
3. Licenses are already in the source code and can be kept in the minified files.

Uglify was a standard for many years but after some research I landed on Terser.
Terser also has support for ES6 so it could be potentially used in a broader application in the future.
It is used by many large js projects, and has 3.6k stars on Github.
I added a build script to package.json so that you can run yarn build:jqueryui
It globs for any non -min .js files and outputs the min file.

The datepicker i18n files are an exception, they don't end in -min. I left them intact and "ignored" that directory in the glob.
This folder would be removed entirely if datepicker gets deprecated so I didn't want to spend extra time on this for now.

It is difficult to compare minified files but if you look at smaller ones like data-min.js you can see the only code difference is the letter used for variable names.
The other big change from the current minified files is the license, which now is copied from the source files and so the wording is slightly different than what was currently in Drupal core.

I manually checked dialogs, autocomplete etc and checked several tests that involve jQuery UI components, such as:

EntityReferenceAutocompleteWidgetTest
CKEditorIntegrationTest
LayoutBuilderTest

If this patch passes all the testbot tests that will also be a good indicator there is no functional change to these minified files.

The previous patch failed due to tests for deprecation warnings on the removed libraries.
Removed the tests for the deleted asset libraries and restored a deprecation message for checkboxradio and controlgroup (maybe this should say "removed in 10.0" now?)

1. This patch looks great already! 👏🙂
2. 🤓It's interesting that this future risk mitigation issue forces us to adopt a JS minifier, which then opens the door to start using that for other core JS in a follow-up.
3. 🤔 Should we remove core/assets/vendor/jquery.ui/package.json?
4. 🤔 Should we remove core/assets/vendor/jquery.ui/AUTHORS.txt?
5. 🐛 We should certainly remove core/assets/vendor/jquery.ui/README.md.
6. 🐛 The patch removes the jQuery UI Accordion plugin, but does not yet remove core/assets/vendor/jquery.ui/themes/base/accordion.css. Similar observation for tooltip and others.
7. 🐛 The patch does not yet remove core/assets/vendor/jquery.ui/effects/*.

8. --- a/core/assets/vendor/jquery.ui/ui/widgets/sortable-min.js
   +++ b/core/assets/vendor/jquery.ui/ui/widgets/sortable-min.js
   […]
   --- /dev/null
   +++ b/core/assets/vendor/jquery.ui/ui/widgets/sortable.js
   […]
   

   The served JS file remains fundamentally the same: slightly different minification, functionally the same. Still no comments in the minified code.

   The source file is added.

   That's exactly what we're doing everywhere 👍

9. +++ b/core/package.json
   @@ -14,6 +14,7 @@
   +    "build:jqueryui": "cross-env NODE_ENV=develop BABEL_ENV=legacy node ./scripts/js/jqueryui-build.js",
   

   👍 A new command for this, excellent!

   🤔 But shouldn't we also run this automatically as part of the yarn build command? It being too slow is not a reason not to do it, because yarn build:jqueryui only takes 1.3–2.3 seconds (on my machine). The entirety of yarn build:js takes about 3 seconds.

   Adding it to build:js still won't slow down watch:js, so live core development will not be slowed down.

   Curious about your thoughts!

   Note that if we don't do this, then we need to update https://github.com/alexpott/d8githooks.

10. +++ b/core/package.json
    @@ -55,7 +56,8 @@
    +    "terser": "^4.3.9"
    
    +++ b/core/scripts/js/jqueryui-build.js
    --- /dev/null
    +++ b/core/scripts/js/jqueryui-terser.js
    
    +++ b/core/scripts/js/jqueryui-terser.js
    @@ -0,0 +1,15 @@
    +  const result = Terser.minify(file);
    

    ⚠️ This is a new dependency. We still need to do the necessary vetting of this package then, just like we did for example for postcss in #3060153: Use PostCSS in core, initially only for Claro or for justinrainbow/json-schema in #2843147: Add JSON:API to core as a stable module.

    Tagging Needs issue summary update for this.

11. +++ b/core/scripts/js/jqueryui-build.js
    @@ -0,0 +1,39 @@
    + * Run build:jqueryui with --file to only parse a specific file.
    ...
    + * yarn run build:jquery -- --file assets/vendor/jquery.ui/ui/widget.js --file
    

    🔎🐛 build:jquery vs build:jqueryui

12. +++ b/core/tests/Drupal/KernelTests/Core/Asset/LegacyLibraryDiscoveryTest.php
    @@ -27,16 +27,6 @@ protected function setUp() {
    -  public function testJqueryUiAccordion() {
    
    @@ -57,244 +47,4 @@ public function testJqueryUiControlgroup() {
    -  public function testJqueryUiDroppable() {
    

    🥳

13. +++ b/core/yarn.lock
    @@ -5202,6 +5215,15 @@ tcp-port-used@^1.0.1:
    +terser@^4.3.9:
    +  version "4.3.9"
    +  resolved "https://registry.yarnpkg.com/terser/-/terser-4.3.9.tgz#e4be37f80553d02645668727777687dad26bbca8"
    +  integrity sha512-NFGMpHjlzmyOtPL+fDw3G7+6Ueh/sz4mkaUYa4lJCxOPTNzd0Uj0aZJOmsDYoSQyfuVoWDMSWTPU3huyOm2zdA==
    +  dependencies:
    +    commander "^2.20.0"
    +    source-map "~0.6.1"
    +    source-map-support "~0.5.12"
    

    ⚠️ The dependencies of terser also need to be vetted.

Thanks @Wim Leers, I'll fix the mistakes you caught and update the patch.

#10.2 I agree! That would be a nice outcome of this work.
#10.3,4,5 I think you're right these 3 files can be removed.
#10.6, 7
My codebase looked correct but when I applied the patch to 9.0.x I saw the problems you found,

+++ b/core/assets/vendor/jquery.ui/ui/keycode-min.js
@@ -1,4 +1,9 @@
diff --git a/core/assets/vendor/jquery.ui/themes/base/accordion.css b/core/assets/vendor/jquery.ui/ui/keycode.js

diff --git a/core/assets/vendor/jquery.ui/themes/base/accordion.css b/core/assets/vendor/jquery.ui/ui/keycode.js
similarity index 19%

similarity index 19%
rename from core/assets/vendor/jquery.ui/themes/base/accordion.css

rename from core/assets/vendor/jquery.ui/themes/base/accordion.css
rename to core/assets/vendor/jquery.ui/ui/keycode.js

rename to core/assets/vendor/jquery.ui/ui/keycode.js
index e096c8c541..c02a6df52b 100644

index e096c8c541..c02a6df52b 100644
--- a/core/assets/vendor/jquery.ui/themes/base/accordion.css

--- a/core/assets/vendor/jquery.ui/themes/base/accordion.css
+++ b/core/assets/vendor/jquery.ui/ui/keycode.js

+++ b/core/assets/vendor/jquery.ui/ui/keycode.js
+++ b/core/assets/vendor/jquery.ui/ui/keycode.js
@@ -1,23 +1,45 @@

@@ -1,23 +1,45 @@
 /*!
- * jQuery UI Accordion 1.12.1
+ * jQuery UI Keycode 1.12.1

Somehow git was interpreting the removal as a rename and put code in the wrong file name in several places. I'll try to get this sorted out.

#10.9 I considered adding it to the build command, but I also thought these files will very rarely change, and, if they do, both the source and minified files would be committed. I hadn't considered the githooks change though.
Your suggestion to add build:jqueryui to the build task should be a low-cost way to solve this question.

#10.10, 13 I'll research this and update the IS.

#10.6 My git diff --find-renames setting was too fuzzy 😅
This patch now has the correct deletions.

Removed the additional jquery ui files:

• package.json
• README.md
• AUTHORS.txt

and some css files which weren't being loaded:

• all.css
• base.css

Added the build command to yarn build,fixed the comment and removed 2 unnecessary requirements from jqueryui-terser.js

The dependencies of terser also need to be vetted.

These 3 dependencies were already in yarn.lock:
commander was introduced in #2869825: Leverage JS for JS testing (using nightwatch) as a dependency of nightwatch / mocha-nightwatch and is currently a dependency of mocha.

source-map is a dependency of Babel, added in #2815077: Adopt airbnb javascript style guide v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib
and PostCSS #3060153: Use PostCSS in core, initially only for Claro

source-map-support is a dependency of babel-register,
also added in #2815077: Adopt airbnb javascript style guide v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib

We still need to do the necessary vetting of this package

I made a start on this in the IS, I wasn't able to find any security policy on their github or website but I can open an issue there if necessary.

#13: like @xjm in #14, I'm confused then why it shows up in package.json if we were already using those dependencies. I do see that they are not being added to core/package.json, but they are being added to core/yarn.lock. How is it possible that these were previously not in our yarn.lock file?

6. Something that's very important is to add documentation (somewhere, any thoughts?) about how to re-minify the JS. Like idiot-proof step-by-step instructions, because this might need to be done by people who don't know a thing about JavaScript (i.e. me, lol) during a security window.

That's part of the reason I raised #10.9. As of #12, yarn build is all you need to do.

But! https://github.com/alexpott/d8githooks/blob/master/pre-commit-8-4 does not run yarn build, it does yarn build:js on JS files specifically.

In #10.9, I actually suggested to add it to yarn build:js, not to yarn build, but I see that I worded it in a very confusing way — sorry about that 🤦‍♂️

I think it needs to be documented in the handbook

Where in the handbook?

Do we know what they used for their minifier?

It looks like they were using a grunt task to minify using UglifyJS,
see: https://github.com/jquery/jquery-ui/blob/master/Gruntfile.js#L53
and: https://github.com/jquery/jquery-ui/blob/master/package.json#L67

https://github.com/gruntjs/grunt-contrib-uglify

I could swap out Terser for Uglify, it is still an active project and remains very popular.
Uglify has similar dependencies:
https://github.com/mishoo/UglifyJS2/blob/master/package.json#L25

Webpack switched to using Terser from Uglify last year, https://github.com/webpack/webpack/pull/8036

To clarify, are all the dependencies already things we used as Babel dependencies, or are there new ones? They do seem to be new entries in the diff of yarn.lock.

These are certainly new entries but they are for specific versions of those packages.
For example, this patch adds an entry for commander@^2.20.0 but there was already commander@2.15.1 and commander@^2.11.0 in the yarn.lock.

Those 3 packages: commander, source-map and source-map-support are also dependencies of mocha and babel which have been part of our dev dependencies for some time.

#14.3 I didn't post an interdiff because there were big problems with the previous match (my own config was set incorrectly so many deleted files were instead renamed).

#14.4 I can restore the AUTHORS.txt
#14.6 This is the page we'd worked on for PostCSS, adding the new yarn commands:
https://www.drupal.org/docs/8/frontend-developer-tools-for-drupal-core
Would that be a good place to add some information - once this is finalized?

I actually suggested to add it to yarn build:js, not to yarn build

I'd added it to yarn build because it was already a "grouping" of other yarn commands-
"build": "yarn build:css & yarn build:js & yarn build:jqueryui",
vs
"build:js": "cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js & yarn build:jqueryui"

where build:js is a specific command to run the es6 transpilation.

I'm happy to make that change if you think adding it in build:js makes more sense.

I don't think we should remove AUTHORS.txt

Reverted the removal.

maybe we could get a JS maintainer's feedback/recommendation to confirm using it

It would be great to have a +1 for Terser from a JavaScript reviewer, especially if it seems like a good candidate for minifying other js files in core.

I added a change record and release notes in the IS

All the latest comments by both @xjm and @zrpnr make sense to me. I've clarified the change record slightly and fixed nits in the issue summary. Added docs to https://www.drupal.org/docs/8/frontend-developer-tools-for-drupal-core (see diff). I've re-applied the patch locally and verified that my remarks from #10 have been addressed.

P.S.: I'm not sure needs JavaScript review is truly necessary here, because it's still the exact same JS. This is a pure release management decision. Still, I totally agree it would be great to have explicit sign-off from a Drupal core JavaScript maintainer. But I propose we timebox it to at most two weeks (that would mean November 6).

Hah, right! 🤓 Thanks for confirming. Removed tag.

That means this is down to reviews that @lauriii and release manager(s) need to do.

Updated issue title to include removing deprecated code.

Is this definitely postponed on #3064049: Replace jQuery UI sortable with Sortable js and #3072906: Deprecate and remove jQuery UI datepicker? Those are pretty close so it probably doesn't matter too much, but trying to understand the dependency. I realise we'll have to fork those two if they don't just get deprecated, but removing the already-deprecated libraries and forking the ones we have to seems independent.

I agree they're independent. And so does @zrpnr. From Slack last night:

catch  17 hours ago
Do we definitely, definitely want to block this on sortable and datepicker, or could we disentangle somehow?

catch  17 hours ago
I guess not really because those will have dependencies on our forked bits of jquery UI so they're interdependent until they're not..

zrpnr  17 hours ago
the current patch keeps datepicker and sortable, removing them could be a followup

wimleers (he/him)  17 hours ago
yep, I don’t think it’s hard-blocked :slightly_smiling_face:

wimleers (he/him)  17 hours ago
Also, Sortable has been RTBC for a while, and I literally just a minute ago pinged the a11y folks for the datepicker issue: https://drupal.slack.com/archives/C2ANFUGGG/p1571846679133700

wimleers (he/him)  17 hours ago
:fingerscrossed: :fingerscrossed: :fingerscrossed: :fingerscrossed: :fingerscrossed:

zrpnr  17 hours ago
datepicker would be especially nice to have for this issue, since the entire i18n folder could be removed

IOW: if this lands before those two land, we'll just need to create follow-up issues to remove them separately, instead of as part of this issue. But I think there is merit to landing this first, despite those two having landed, because this does the bulk of the work; those follow-ups would be trivial.

#27: ehm … that tag just got removed based on your comment in #22.

Re-rolled now that #3072906: Deprecate and remove jQuery UI datepicker is in 9.0.x
That issue removed datepicker so this patch is now updated without adding datepicker.js source and re-minifying datepicker-min.js.

The interdiff has a glitch around base.css which changed between these patches, however just like #20, base.css is still removed in this patch.

Reviewed the patch,

First yay for deleting code :)

1. Terser: happy with it. could be used later for core js once browser support makes it possible to ship ES6 as-is.
2. Agreed with adding the jquerui command to the "build" one
3. New build scripts are straightforward, +1
4. -200+ lines removed from core.libraries.yml, reviewed the dependencies, didn't see any missing ones, dialog and ckeditor config still works.

Looking forward to the other issues replacing jquery ui :)

Re-rolled against 9.0.x, really exciting to be able to remove jQuery UI sortable now too!
The jQuery UI touch-punch library was only attached in CKeditor in core, now it has no other core dependents and can be removed too.

Removed:

• sortable-min.js
• sortable.css
• core/assets/vendor/jquery-ui-touch-punch

Removed core/jquery.ui.sortable and core/jquery.ui.touch-punch from core.libraries.yml
The source for jquery.ui.sortable is not added in 37.

For reference, since this is a huge patch, the diff in the lsdiff between 32 and 37:

> - a/core/assets/vendor/jquery-ui-touch-punch/jquery.ui.touch-punch.js
9a11
> - a/core/assets/vendor/jquery.ui/themes/base/sortable.css
89,90c91
< ! a/core/assets/vendor/jquery.ui/ui/widgets/sortable-min.js
< + b/core/assets/vendor/jquery.ui/ui/widgets/sortable.js
---
> - a/core/assets/vendor/jquery.ui/ui/widgets/sortable-min.js

The #37 looks good.

Since this is a highly atypical patch, let's look at it from a high level for a moment.

#32:

99 files changed, 9650 insertions, 1206 deletions.

#37:

100 files changed, 8087 insertions, 1255 deletions.

Great!

The patch in #37 is smaller, deletes more, inserts less, because it no longer needs to fork jquery.ui.sortable 🥳

Moving back to RTBC 👍

1. +++ /dev/null
   index fd04188ab8..0000000000
   --- a/core/assets/vendor/jquery.ui/README.md
   

   Should we replace this with our own README.md? It would be great if we could mention why we have copied the files and how to generate the minified files.

2. +++ b/core/package.json
   @@ -8,12 +8,13 @@
   +    "build:jqueryui": "cross-env NODE_ENV=develop BABEL_ENV=legacy node ./scripts/js/jqueryui-build.js",
   

   What is NODE_ENV=develop? Shouldn't this be NODE_ENV=development?

3. We are missing all of the jQuery UI test coverage from the main package. Not having those tests in our codebase will increase the cost of maintenance and will lead to an increased risk of causing regressions.

4. +++ b/core/assets/vendor/jquery.ui/ui/version.js
   @@ -0,0 +1,17 @@
   +return $.ui.version = "1.12.1";
   
   +++ b/core/assets/vendor/jquery.ui/ui/widgets/autocomplete.js
   @@ -0,0 +1,682 @@
   +	version: "1.12.1",
   
   +++ b/core/assets/vendor/jquery.ui/ui/widgets/button.js
   @@ -0,0 +1,386 @@
   +	version: "1.12.1",
   
   +++ b/core/assets/vendor/jquery.ui/ui/widgets/checkboxradio.js
   @@ -0,0 +1,286 @@
   +	version: "1.12.1",
   
   +++ b/core/assets/vendor/jquery.ui/ui/widgets/controlgroup.js
   @@ -0,0 +1,298 @@
   +	version: "1.12.1",
   
   +++ b/core/assets/vendor/jquery.ui/ui/widgets/dialog.js
   @@ -0,0 +1,940 @@
   +	version: "1.12.1",
   
   +++ b/core/assets/vendor/jquery.ui/ui/widgets/draggable.js
   @@ -0,0 +1,1250 @@
   +	version: "1.12.1",
   
   +++ b/core/assets/vendor/jquery.ui/ui/widgets/menu.js
   @@ -0,0 +1,673 @@
   +	version: "1.12.1",
   
   +++ b/core/assets/vendor/jquery.ui/ui/widgets/mouse.js
   @@ -0,0 +1,226 @@
   +	version: "1.12.1",
   
   +++ b/core/assets/vendor/jquery.ui/ui/widgets/resizable.js
   @@ -0,0 +1,1201 @@
   +	version: "1.12.1",
   

   What should we do with the version numbers?

What should we do with the version numbers?

I think we should keep the version numbers as is. It's very unlikely but not impossible that jQuery UI could get a new release, so it's good to have an easy record of where we forked from. We should probably open a follow-up to decide if we keep them and if we'd change them if we make our own changes though.

#44++

On #43.3 I think we could handle the tests in a follow-up - i.e. as a pre-requisite to making changes, but not to doing the initial fork.

#46: What if we document how to run jQuery UI tests in this issue (including verifying that that's actually possible), and then file a follow-up to check if it's at all feasible to make DrupalCI run those tests for us?

(@xjm gave a thumbs up to this proposal in chat.)

In the current jQuery UI repo, they use grunt to run the qunit tests,
https://github.com/jquery/jquery-ui/blob/master/Gruntfile.js#L217
This uses grunt-contrib-qunit
https://github.com/jquery/jquery-ui/blob/master/Gruntfile.js#L217

which spins up a phantomJS instance for each of the named html pages in the folders in tests/unit
For example, button.html has the markup for the qunit test as well as the jQuery UI component, it loads files from the external folder such as jquery-simulate or requirejs. The tests are run by bootstrap.js.

That file requires many more external files and fires off PhantomJS.

I didn't think adding in all these old requirements would be a good thing, and untangling the qunit tests from phantomJS and grunt seemed like a lot of work. Nightwatch seemed like an excellent alternative, the qunit tests themselves are not complicated, mostly just running some javascript and checking for classes, focus, page elements etc.

There are a lot of qunit tests however, I got through button and most of autocomplete but I thought I should post a work in progress before I spend too much more time on this in case it doesn't seem like a good direction or is better for a follow-up.

I picked Nightwatch over FunctionalJavascript testing with Selenium because it is much easier to execute arbitrary javascript and then make assertions on the page.

This patch has the new nightwatch tests and a test module for providing the markup and pages.

If we're reworking the tests in another issue I think that leaves #43.1 to address. For me it would also be fine to do that in a follow-up to be honest although we should link to it from here.

OK I've opened three follow-ups - for the nighwatch tests, for replacing the README (which should include information about the Nightwatch tests), and for figuring out a future version numbering system for if we need it.

#3093173: Improve README for forked jQuery UI components
#3093172: Test coverage for forked jQuery UI components
#3093175: Document version numbering system for forked jQuery UI components

IMO these are all non-blocking issues - the patch as it stands does not introduce any new technical debt, but moves technical debt from jQuery to us, and the follow-up issues show the extent of that technical debt - however all three follow-up issues only become a problem if we have to change these files. If we it turns out we don't need to backport any jQuery UI security releases before we're able to fully replace the components, we actually won't miss test coverage, a version numbering system, and better docs on how to make changes.

(edit, to clarify I re-RTBCed #37. #48 is the draft test coverage which has been uploaded to the spin-off.

Moving my earlier feedback to follow-up issues seems fine. Since Drupal 9 doesn't have security coverage before Drupal 9.0.0, we probably don't have to make changes to these files in a while which will give us some time to work on the follow-ups.

Another question that I was thinking was which coding standards should the forked code be following? Should they follow Drupal coding standards, jQuery UI coding standards or no coding standards at all? Also, how are we going to enforce these? If we are planning to follow jQuery UI coding standards, should we at least find where jQuery UI coding standards are documented and see if there's some predefined tooling for linting?

Another question that I was thinking was which coding standards should the forked code be following? Should they follow Drupal coding standards, jQuery UI coding standards or no coding standards at all? Also, how are we going to enforce these? If we are planning to follow jQuery UI coding standards, should we at least find where jQuery UI coding standards are documented and see if there's some predefined tooling for linting?

IMHO we shouldn't change a single character in their code right now because it's not materially relevant. We can bikeshed that in a follow-up issue.

Furthermore, there is a strong argument to never deviate from whatever coding standards they were using, because it would complicate porting any security patches from upstream.

IMHO we shouldn't change a single character in their code right now because it's not materially relevant. We can bikeshed that in a follow-up issue.

+1 to a follow-up.

Furthermore, there is a strong argument to never deviate from whatever coding standards they were using, because it would complicate porting any security patches from upstream.

I agree. I think we just need to agree on something so that if we have to work on a security issue, we already know what these policies are. Maybe it would be fine if we say we don't care about coding standards on the jQuery UI forked code but then it means we don't accept any feedback on coding standards because otherwise, it leaves the door open to everyone give feedback on random coding standard related matters.

#43.2

What is NODE_ENV=develop? Shouldn't this be NODE_ENV=development?

That's right, the env var isn't used in the new script but it should certainly be consistent with the others. This patch makes the change in package.json.

Since the testing work is moved to a follow-up issue I based this change off the previous patch in #37 which catch RTBC'd in #51.

+1 for keeping the source identical to what is currently in their github, and not apply Drupal coding standards to it.

Maybe it would be fine if we say we don't care about coding standards on the jQuery UI forked code but then it means we don't accept any feedback on coding standards because otherwise, it leaves the door open to everyone give feedback on random coding standard related matters.

This is tricky, it looks like the entire vendor folder isn't checked by testbot for coding standards now.
Although this is now "forked" into Drupal there is still a chance of upstream updates. Any patches from this community could get feedback about matching the existing formatting.

I accidentally named the interdiff as a .patch in the previous comment

This is tricky, it looks like the entire vendor folder isn't checked by testbot for coding standards now.

I think that makes it not tricky, because that means this requirement is already taken care of in HEAD :) The contents of core/.eslintignore confirm this:

assets/vendor/**/*

Hence Backbone, CKEditor, jQuery, and so on, including jQuery UI. jQuery UI continues to live in core/assets/vendor/jquery.ui. This is semantically not 100% correct: a forked dependency in principle should live in core/assets/, not in core/assets/vendor. But no JS lives there; all of the JS owned by Drupal core lives in core/misc, which is hardly better. Arguably, this is semantically still vendor JS, it's just that we're now signing up for maintenance if and only if security patches need to occur that the jQuery UI team does not provide. Continuing to consider it as vendor JS seems appropriate then. This also addresses @lauriii's feedback that we should not accept any feedback on coding standards about this code: we already never accept such feedback about vendor code.

Per #53, #54 and #55, we don't need a follow-up for coding standards anymore.

But I still can't remove needs followup because #41 is why that tag was added originally, and that has not yet been addressed. Could you tackle that too, @zrpnr?

Continuing to consider it as vendor JS seems appropriate then. This also addresses @lauriii's feedback that we should not accept any feedback on coding standards about this code: we already never accept such feedback about vendor code.

This sort of does address my concerns but I'm not sure whether it fully does. I'd like to clarify that I'm not concerned about what coding standards the code is using at the moment. Even more so when it comes to making improvements to the coding standards of the existing code. What I'm actually concerned about is, whether we know and understand the coding standards we're supposed to follow when we make security fixes or some other critical changes. We could say it's up to the person working on the code to decide how the code gets styled if we're okay with that.

But I still can't remove needs followup because #41 is why that tag was added originally, and that has not yet been addressed.

I updated the docs for Terser: https://www.drupal.org/core/dependencies#terser per #42
and created an issue to check the jquery.ui folder with d8githooks: https://github.com/alexpott/d8githooks/issues/28 per #41

I created #3094226: Determine what coding standards to apply to jquery ui source code to address the concerns about coding standards for the jQuery UI source code.
Updated the issue summary with a full list of the files removed/added/changed in this patch for comparison.

I re-formatted the commands in the issue summary so that they are easier to run if anyone needs to regenerate the patch. I confirmed with these steps that the patch has been generated according to these steps. There was a minor difference in the formatting of one file which we might want to consider fixing still. Interdiff attached.

Should we remove the deprecation from jquery.ui.checkboxradio and jquery.ui.controlgroup given that they are still used, and won't be removed from Drupal 9.0.0?

1. Can we remove form-min.js even though it seems to be used by form-reset-mixin.js?
2. Can we remove labels-min.js and escape-selector-min.js even though those seem to be used by selectmenu.js and checkboxradio.js?

Thanks for going over this in detail @lauriii, and improving the patch commands in the IS.

#62

There was a minor difference in the formatting of one file

Looking at this now I'm surprised it is only 1 file, I think my IDE was converting tabs to spaces?
The other source files in the jQuery UI github also seems to be using tabs as well. Since we agreed to keep the source formatting identical, I should probably re-add all of these source files using the step in the IS.

#63 I wondered if we could remove checkboxradio and controlgroup as a follow-up rather than removing the deprecation notice, those 2 are only used by button and are marked deprecated.
It could be a good first change to the jQuery UI codebase to remove that usage from button.
https://github.com/jquery/jquery-ui/blob/1.12.1/ui/widgets/button.js#L344
https://github.com/jquery/jquery-ui/blob/1.12.1/ui/widgets/button.js#L364

In the qunit tests everything related to checkboxradio and controlgroup for button are in a separate "deprecated" file.
If that doesn't seem likely then yes unfortunately maybe we should remove the deprecation warning in Drupal 9.

#64.1 The form module is defined in https://github.com/jquery/jquery-ui/blob/1.12.1/ui/form-reset-mixin.js#L21 but I don't see where it is explicitly used so it doesn't look like it would trigger an error message. It seems to be for IE8 support, https://github.com/jquery/jquery-ui/blob/1.12.1/ui/form.js so it could probably be removed safely.
#64.2 selectmenu is removed as part of this patch so that dependency isn't a problem, however labels and escape-selector are both requried by checkboxradio. Labels also isn't called directly but if escape-selector is missing there will be an error in the console, see #2926155: Follow-up to #2809427: update from jQuery UI 1.11.4 to 1.12.1 forgot to list some new JS files in *.libraries.yml

In general is it better to do this kind of cleanup here or in a follow-up?
Any of these removals would require changes to the source files- at minimum to remove from the AMD define functions.

#62

There was a minor difference in the formatting of one file which we might want to consider fixing still.

Fixed the spacing by copying the file directly.

#41

one followup we should do is post an issue in the committer tools

Opened https://github.com/alexpott/d8githooks/issues/28 and created a PR.

Working on d8githooks I realized this needs a --check option for that pre-commit to work, added an additional js file that matches the check for es6 files. It returns an error if the compiled code does not match the existing file.

I have done a test with patch in #66.
The dialog and autocomplete are all works for me.
Testing video is attached.

1. Confirmed that the steps listed in the issue summary result into #66 ✅
2. Documentation has been added to https://www.drupal.org/core/dependencies#dev ✅

3. +++ b/core/package.json
   @@ -8,12 +8,13 @@
        "watch": "yarn watch:css & yarn watch:js",
   

   We might want to consider adding a watch command for this but I don't think this is needed for getting this patch in ✅

4. +++ b/core/package.json
   @@ -55,7 +56,8 @@
   +    "terser": "^4.3.9"
   

   Dependency evaluation has been added to the issue summary for the new dependency ✅

5. +++ b/core/scripts/js/jqueryui-build.js
   @@ -0,0 +1,46 @@
   + * assets/vendor/jquery.ui/ui/plugin.js</caption
   ...
   + * @example <caption>Check if all files have been compiled correctly</caption
   

   Should this also have a > in the closing tag? 🔬👁

6. #64 / #65: I'm in favor of moving removing those files to a follow-up. It also seems like a less risky option than committing the patch as it is given that we have some files referencing non-existing files at the moment.

It looks like the only thing left is to fix the comment in #69.5

#69.6 I created #3098489: Remove deprecated jQuery UI library definitions to address the removal of the files in #64and #65

With these source files in core they can be modified to trim even more unused code without causing any errors.

I misunderstood #64, those files should not have been removed here since they are depended on by remaining files.
Restored those 3 minified files: labels-min.js, form-min.js, escape-selector-min.js,
then added the source: labels.js, form.js, escape-selector.js and re-minified.

Also updated the IS for those 3.

+++ b/core/core.libraries.yml
@@ -411,14 +410,11 @@ jquery.ui:
-    assets/vendor/jquery.ui/ui/form-min.js: { weight: -11, minified: true }
-    assets/vendor/jquery.ui/ui/labels-min.js: { weight: -11, minified: true }
...
-    assets/vendor/jquery.ui/ui/escape-selector-min.js: { weight: -11, minified: true }

@@ -741,118 +555,6 @@ jquery.ui.resizable:
-jquery.ui.selectmenu:
-  version: *jquery_ui_version
-  license: *jquery_ui_license
-  js:
-    assets/vendor/jquery.ui/ui/form-reset-mixin-min.js: { minified: true }
-    assets/vendor/jquery.ui/ui/widgets/selectmenu-min.js: { minified: true }
-  css:
-    component:
-      assets/vendor/jquery.ui/themes/base/selectmenu.css: {}
-      assets/vendor/jquery.ui/themes/base/button.css: {}
-  dependencies:
-    - core/jquery.ui
-    - core/jquery.ui.menu
-    - core/jquery.ui.position
-    - core/jquery.ui.widget
-  deprecated: *jquery_ui_unused_deprecated
-

Should we revert these changes too?

Re: #73 This reverts the includes for jquery.ui:, those should be there since the files are not being removed. I don't believe jquery.ui.selectmenu: needs reverting as it's still among the libraries being removed and I did a quick grep in jQueryUI to confirm it's not a dependency anywhere that may have not been defined in core.libraries.yml.

Thanks for catching those @lauriii and for updating the patch @bnjmnm !
You're right that selectmenu was not on the list to revert, it was marked deprecated and didn't have any dependents so it is safe to leave it removed.

+++ b/core/core.libraries.yml
@@ -410,11 +410,14 @@ jquery.ui:
+    assets/vendor/jquery.ui/ui/form-min.js: { weight: -11, minified: true }
+    assets/vendor/jquery.ui/ui/labels-min.js: { weight: -11, minified: true }
...
+    assets/vendor/jquery.ui/ui/escape-selector-min.js: { weight: -11, minified: true }

These 3 files added back in #72 are the only ones that needed to be restored in core.libraries.yml

Thanks for pointing out that! You're right, we can remove jquery.ui.selectmenu. 👍I think we're done here, let's move on to working on the follow-ups. 🎉

Committed 75ac95e and pushed to 9.0.x. Thanks!