Remove usages of the JavaScript ES6 build step, the build step itself, and associated dev dependencies

MR is too big for gitlab, see the raw patch: https://git.drupalcode.org/project/drupal/-/merge_requests/2219.diff

IMO the easiest way to review that would be to apply the patch locally to see which files changed outside of all the renames

I stretched the scope a little to be able to remove all of our babel deps (not using import in nightwatch file, but old school require).

hopefully we can get #1014086: Stampedes and cold cache performance issues with css/js aggregation going to be able to solve comment removal down the line.

Kept the scripts around, will do the package cleanup in a different issue.

since the lint/prettier configuration changed a few files got caught up and were reformatted.

Applying the MR as a patch and then reviewing on the CLI with git diff --break-rewrites makes it significantly easier to see what's going on for me.

yes, they add 59 lint errors if we don't exclude them

Another idea @bnjmnm raised was to rename our libraries.yml definitions to reference the es6.js files, which would keep the source files same. This would create some issues with code referencing the library definitions, such as libraries-override API in themes. There might be ways to workaround that.

There is a fairly easy solution to keep libraries override working, see #2484623: Move all JS in modules to a js/ folder. That wouldn't help with the cognitive dissonance of naming es6 files that are not just es6 anymore.

Follow-up created #3279183: Fix eslint errors in newly linted files

Sorry, but as non-JS developer I'm now a little bit confused. Not sure how this is impacting Drupal core JS development. I think this deserves at least:

• A change record explaining the impact on Drupal core JS development.
• Preparing documentation changes
• Changes to https://www.drupal.org/node/2815083
• Marking some issues, such as https://www.drupal.org/project/drupal/issues/2957390, as "Won't fix"

@claudiu.cristea, we are currently working on a blog post for the core blog that pre-announces this change and that I believe will help with some of your questions.

Created a new reviewable merge request with everything except the file renaming: https://git.drupalcode.org/project/drupal/-/merge_requests/2482/

End of august we should go from this MR and add the file rename, that will simplify things and prevent having to do a painful reroll.

There is a bunch of formatting changes because those files were not picked up previously by eslint since they were not named .es6.js, now they are and those changes are needed so that yarn lint:core-js-passing (and commit checks) work as expected.

On top of this MR only the file name changes are required to be green (hopefully).

I just tried to commit #3294720: The attachBehaviors() for document is only called after Big Pipe chunks are processed to 10.1.x, and it failed with this:

$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /home/catch/www/drupal/core/modules/big_pipe/js/big_pipe.es6.js
⚠️  yarn `build:js` command is deprecated in drupal:9.4.0 and will be removed from drupal:10.0.0. This command is no longer needed in Drupal 10.0.0 once https://www.drupal.org/project/drupal/issues/3278415 is committed.️

We either need to undo the deprecation, or commit this earlier.
edit: this was a false alarm, it was something else.

The comment removal that the build step does makes quite a significant difference to the filesizes we send to browsers - for example ajax.es6.js is 59kb and ajax.js is 22k.

We can claw that back (and more) in #3302755: On-the-fly JavaScript minification though.

First I want to note that with Umami the change is limited to authenticated users and there is no meaningful difference as an anonymous user when visiting the site.

This is a crucial distinction! 👍

(This is a question I raised in the discussion that @lauriii mentions at the end of #33.)

Detailed impact

So the detailed consequences are:

1. ~13% more bytes on the wire (but can be cached client side thanks to Cache-Control response headers)
2. ~13% more bytes to (lex and) parse and compile into bytecode (but cached on disk too)
3. 0% more bytes to execute.

Sources for the claim in the second point: V8 (Blink, used in Chrome/Edge/Opera/Chromium/…), SpiderMonkey (Gecko, used in Firefox), JavaScriptCore (WebKit, used in Safari).

Real-world end user impact

Translated to real-world end user impact:

anonymous users

• first visit: virtually no impact
• repeat visit: ZERO impact

authenticated users

• first visit: 13% more bytes to transfer, parse and compile — ZERO impact on JS execution
• repeat visit: ZERO impact

Conclusion

I think it's been sufficiently demonstrated that the front-end performance regression here will have a minimal real-world impact. Even if it were to jump to 20%, that'd still be limited to some phases of the first visit.

Given that:

1. this unblocks a massively less painful DX
2. we have a strong commitment from long-term contributors in the Drupal front-end space to finally bring JS minification to Drupal core in #3302755: On-the-fly JavaScript minification (far simpler now that #1014086: Stampedes and cold cache performance issues with css/js aggregation has landed after 11.5 years)

I think this is now ready to land 😊 I see that this still needed a change record — I wrote one: https://www.drupal.org/node/3305487.

The MR doesn't do the renames so I don't think this is actually committable yet.

In #3302755: On-the-fly JavaScript minification we are now leaning towards providing pre-minified core JS files, so we only have to minify un-minified contrib/custom JavaScript at runtime. This means retaining some kind of build step to call the minifier. So should we hold off on this for now, in the event that we have to revert it and re-add the build step again?

Back when we talked about this first #2809281: Use ES6 for core JavaScript development one concern was that the transpiled version should be reviewable to make sure we didn't introduce malicious code by mistake. Since then we introduced the commit check script that makes sure files are processed properly and nothing "extra" was introduced so it might be possible to have the minified file committed. We'll have to remove diffs of minified files to avoid ugly merge requests on gitlab though.

Ideally the minification step is done by CI at some point and not a responsibility of the committer/contributor. Starting to get off topic, but wanted to add some more context as to why this wasn't done until now.

I've just opened #3308122: Pre-minify core JavaScript. If we do that, this issue will be won't fix/outdated.

Ideally the minification step is done by CI at some point and not a responsibility of the committer/contributor.

But that is not realistic, since it'd require CI to be able to commit the result to git.

Or … I guess you mean it should be a responsibility of the packaging scripts. But that'd mean that it'd only happen for releases, not for arbitrary commits in branches. Meaning that we'd have different code running in the tip of a branch vs in a tag. Meaning that lots of tests would need to change.

So IMHO @longwave's conclusion is right. Though I'm really curious to read more opinions on this, particularly @lauriii's since he opened this 🤓

it should be a responsibility of the packaging scripts

Composer can also be configured to directly check out releases from git, so IMHO whatever code we want to ship to end users must exist in the repo. I guess it's possible to provide different code for source (git) vs dist (tarballs) for a tag, but that feels like it would get very confusing very quickly.

So the reason to remove the duplication of files was to improve contributor experience, reduce the need for rerolls and remove a lot of JS dependencies from core package.json.

If we minify them we improve performance but we keep the contributor experience the same, or even slightly worst since we'll need the additional sourcemap file, so one change in a JS file means the MR will have 3 file changed.

Minifying core JS is pretty trivial, a couple of lines changed in the build script and that would do it nicely. Especially since we don't need babel anymore we can still remove a bunch of dependencies.

• Keeping a single file with the source that is run though peast for minifications has better contrib experience, lower file size compared to what we ship now (there are no comments but we still have all the whitespace) at the expense of processing time when aggregating files.
• Shipping minified JS has less good contrib experience than today, lowest file size possible, at the expense of contributors.

I'm on the side of making computer work, even if it's repetitive/or non optimal. It's just not worth it that the first contribution of someone on core JS is most likely a CI error saying the .js file needs to be updated, or that they changed the .js file but missed that it's the .es6.js that needs to be updated.

So I would still strongly suggest we get rid of the build step, and figure out another flow that makes the computer do all the work later.

and that other flow should be made to work with contrib from day 1 so they don't have to reinvent the wheel. if we remove the comments core files are not the worst, we already ship minified jquery, ckeditor*, etc. and again if the site has tracking/ads all this discussion is moot since they're by far the worst offenders performance-wise.

What is the advantage of committing the products of a build script to the git repo? For me, it seems preferable to keep the git repo clean and require that developers run a build command if they are working from a raw git checkout (e.g. iirc, Wordpress works this way). And of course the build command would also need to run during packaging, and by CI for purposes of running tests.

Seems like the only advantage of putting minified JS in the git repo is that a developer (or test bot) working with a raw git checkout doesn't have to run any extra commands, aside from composer install? But honestly, running a build command seems pretty typical for developers, it's hard to see how it would cause too much burden. Theoretically, third-party source JS could also be removed from the repo, in favor of being installed by npm/yarn.

@mfb it's because we want to pre-minify core JavaScript for serving to browsers, and we want this to be available via tagged releases of core (i.e. not require all Drupal users to have node installed).

Discussed this in slack with nod_ and lauriii, and we all agreed the following should work:

1. Do this issue - remove the build step and stop having .es6.js files. This means an increase in core js file sizes in 10.0.x but it's just reverting an unintended side-effect of the build step.

2. Try to land #3302755: On-the-fly JavaScript minification in 10.1.x which will improve the filesizes back under what they've been in 9.x

3. Try to add pre-minification separately in #3308122: Pre-minify core JavaScript, which would operate in addiiton to #2, details TBD.

I think that means we mostly need a re-roll of the MR that addresses #36 here.

Manual patch + the result of the command below:

for file in $(find . -type f -name "*.es6.js"); do mv $file $(dirname $file)/$(basename $file .es6.js).js; done;

patch generated with git diff HEAD --break-rewrites

guess we still need to run prettier so that things are formatted properly in core/scripts/* even if they're not linted yet.

A patch for the testbot since it has trouble with renaming file over an existing one. Review #47, not this one.

+++ b/core/.eslintignore
@@ -1,14 +1,15 @@
-*.js
-!*.es6.js
-!modules/ckeditor5/js/ckeditor5_plugins/**/*.js
-modules/locale/tests/locale_test.es6.js

Can ckeditor5_plugins and NightWatch really be removed from here? Possible I'm mis-reading the patch too.

so previously the configuration ignored all .js files (including es6.js), then force added the .es6.js files, nightwatch, and ckeditor files to eslint.

now we don't exclude anything so we can remove all those rules to achieve the same thing.

@mfb it's because we want to pre-minify core JavaScript for serving to browsers, and we want this to be available via tagged releases of core (i.e. not require all Drupal users to have node installed).

@catch couldn't the packaging script do the minification? And in addition there could be a simple build command that developers and test bots run if they are using a raw git checkout.

We can discuss that in #3308122: Pre-minify core JavaScript :)

1. +++ b/core/.prettierignore
   @@ -1,2 +1,6 @@
   -modules/locale/tests/locale_test.es6.js
   +assets/vendor/**/*
   +node_modules/**/*
   +**/build/**/*
   +**/js_test_files/**/*
   +modules/locale/tests/locale_test.js
   

   Let's also add scripts/** here so we don't have to update core/scripts/js/* files in this patch — that is out of scope and can happen in any future commit after beta 🤓

2. +++ b/core/package.json
   @@ -8,8 +8,8 @@
        "build:js": "cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js",
   

   We should still remove this to meet the issue scope's goals — but I'm glad you didn't do it already because it made #47 a lot easier to review! 😄

3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php
   @@ -418,8 +416,7 @@ protected function getClassyHash($type, $file) {
   -        'media_embed_ckeditor.theme.es6.js' => 'd9fa9e008aff8d4fb0401083bab0a1ad',
   -        'media_embed_ckeditor.theme.js' => 'b2e705b4503be407a35692b272c7ed6a',
   +        'media_embed_ckeditor.theme.js' => 'd9fa9e008aff8d4fb0401083bab0a1ad',
   

   👍 This is safe because neither *.library.yml nor theme *.info.yml definitions/overrides/alters targeted the *.es6.js files!

Addressed 1, 2 from #53

I decided to remove babel-related things from nightwatch since it is not necessary if we use the old school "require" instead of ES module style imports. With that I was able to remove from package.json:

    "@babel/core": "^7.0.0",
    "@babel/preset-env": "^7.0.0",
    "@babel/register": "^7.7.7",
    "babel-plugin-add-header-comment": "^1.0.3",
    "cross-env": "^7.0.2",

It's that much less to care about. The only thing that brings in babel is "postcss-header" if we get rid of that we don't have to worry about babel anymore. (edit) saw that #3261163: Update to PostCSS 8 means @babel/core is not needed anymore, after those two patches, we'll be pretty much babel-free :)

1. #53 is indeed fully addressed.

2. +++ b/core/package.json
   @@ -36,9 +32,6 @@
   -    "@babel/core": "^7.0.0",
   -    "@babel/preset-env": "^7.0.0",
   -    "@babel/register": "^7.7.7",
   

   🥳 Fewer dependencies to worry about!

3. +++ b/core/tests/Drupal/Nightwatch/globals.js
   @@ -1,20 +1,17 @@
   -export const drupalDbPrefix = null;
   -export const drupalSitePath = null;
   

   🐛 There are still references to these in core/tests/Drupal/Nightwatch/Commands/drupalInstall.js.

   Also: isn't this an API change? 🤔

as long as there is no error I'd let it go. That file is a bit of a mess of old school export (module.exports and "export" in the same file) So I'm not convinced they're are actually doing anything at the moment.

They're set explicitly in drupalInstall and that's executed before any other command so there won't be a problem or undefined variable or something. Would be a more of a concern if those variables had any documentation but that's not the case.

#56: 👍

🚀

patch for 10.1.x after #3270438: Remove CKEditor 4 from core

patch for 10.0.x after #3270438: Remove CKEditor 4 from core

seems like patch applies to both 10.0.x and 10.1.x so yay.

Committed/pushed to 10.1.x and 10.0.x, thanks!

Going to mark #2957390: Use ES6 for contrib and custom JavaScript development as outdated.

Can we get a release note?

Also I am confused -- I thought we had agreed in #d10readiness earlier in the week to actually wait to do this until we had minification to mitigate the perf regression?

Also I think we would need massive docs updates for this. Thanks!

Since this is a developer facing change I'm not sure it should have a release note - would think the change record would be enough.

Started updating the docs: https://www.drupal.org/about/core/policies/core-change-policies/frontend... (the way to install yarn has been updated too)

updated the link to the doc in the https://www.drupal.org/about/core/blog/javascript-build-process-removed blog post.

Copied the release note from #3278246: Deprecate core/scripts/js/babel-es6-build.js for removal from 10.0.x but updated the link to point to the blog post instead, but also agree with @catch that a release note is probably not needed given this is relevant to core developers only.

The docs updates linked in #66 look good to me, not sure what else is needed, if anything?

Yes I think we're OK here. Marking fixed and untagging. Since we already have a release note, leaving that tag on, we can drop it from the actual release notes if we decide it's not necessary after all.

I published the CR.