Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
In #3302755: On-the-fly JavaScript minification, we eventually came up with the following plan:
1. Pre-minify all core JavaScript, and add {minified: true} to the asset definition.
2. Minify any remaining JavaScript (in practice this will be js from contrib/custom modules and themes) on-the-fly using Peast.
Steps to reproduce
Proposed resolution
We already have foo.es6.js files and foo.js files which are generated by the build step. Modify the build step so that it removes whitespace and comments from the foo.js files. Add {minified: true} to the library definition for every file that relies on the build step.
Remaining tasks
It would be good to compare individual file sizes vs. the Peast numbers in #3302755: On-the-fly JavaScript minification.
Comments
Comment #2
catchComment #4
lauriii+1 for this issue and for closing #3278415: Remove usages of the JavaScript ES6 build step, the build step itself, and associated dev dependencies.
@catch mentioned on Slack that we could rename .es6.js to .source.js which I think makes sense. So long as that's allowed in a minor release, I think we could leave that to a follow-up. Ideally this would be done by CI for CI runs and releases but I think that also can be a follow-up, as long as core includes source maps for the minified files.
Comment #5
Wim LeersBut does it really make sense to have
*.source.jsfiles? Wouldn't it make more sense to have*.jsand then have the build script generate*.min.js?Wouldn't it then be better to not close #3278415: Remove usages of the JavaScript ES6 build step, the build step itself, and associated dev dependencies, but instead close this issue and repurpose that issue that already has had a lot of discussion?
EDIT: what I'm trying to say is that the end result is different, it's simpler than what #3278415 would've done, but we still need to do most of the "mechanics" of #3278415 even if we go this direction? 🤔
Comment #6
catchThat's unfortunately not straightforward.
All of our library definitions currently reference the .js files. This means that any implementation of
hook_library_info_alter()orlibraries_overridethat's removing or replacing an asset references the .js filename. If we change all of core's library definitions to point to .min.js instead, we'd break all those alters. It is theoretically possible to write a bc layer for libraries-override because it's declarative, but not for hook_library_info_alter(). The filenames are not part of the public API so it would technically be allowed, but in practice it would break a lot.We'd need a release branch (or even separate repo?) to commit those changes to (like 10.0.0-release) prior to tagging, since tag === release. That doesn't seem impossible to make that kind of change, but it does seem quite far off.
Comment #7
Wim LeersArgh — that makes sense 😬 But … that's gonna be pretty confusing too IMHO. Not sure we can do anything about it though. 🤔
And actually … any
hook_library_info_alter()that changes the JS files used in an asset library would have had its JS assets get minified today (if they haveadvagginstalled), becauseminified: false. But we're going to change it tominified: true. That means that such alterations will usually still haveminified: true(unless they are overriding all that today too), and hence the JS will not get minified, even though it should.That's of course far less severe than an override failing altogether; it just means a slight front-end performance regression.
So … yeah, you've convinced me of
*.source.jsto succeed the current*.es6.js😢Comment #8
catchSee also nod_ in #3278415-41: Remove usages of the JavaScript ES6 build step, the build step itself, and associated dev dependencies which would make this postponed on a js-contributor-friendly solution and we'd just live with the Peast time on aggregate misses for now (also fine with me if no significant objections from others).
Comment #9
longwaveSo, thinking about this some more. I proposed keeping the build step to avoid minifying on the fly. But is minifying on the fly *really* that much of a performance hit? We can cache the output of minification so we only have to do it once per file, and then aggregation should be a separate step that uses the cached minified output. Only the minification has a performance impact, aggregation takes the same time whether the files are minified or not.
I guess before we go down this route we should figure out:
1. how much smaller code we can produce with JS minifiers vs Peast
2. how long it takes for a cold cache minify of all core JS (or at least a representative subset)
Comment #10
Wim LeersIn theory even less!
… d'oh, of course!
And if we use
hook_rebuild()+hook_cache_flush(), we can do pretty much all of that at cache rebuild time … wouldn't that be a far better trade-off? 🤓Comment #11
nod_yes, i like that very much.
Comment #12
catchI thought about doing minification per-file while working on the Peast patch, but the idea I had was to write the output to a file somewhere, which would mean a lot of reading/writing from those files and having to account for if one is missing. But... we can just cache the string in the cache system! And then if we do that in the JsOptimizer class, it's just a case of having some rebuild logic that gets all the core assets and runs the JsOptimizer on them up front to pre-warm.
The only thing though is we don't want that rebuild logic to block serving requests (or aggregates), so it really just needs to be a pre-warm step rather than blocking - we'll be minifying admin assets that might not be requested, while the front end should already be trying to serve requests to anonymous users that might only need one js file from a theme or similar. It'll be quicker to do that one file than wait for all minification to finish. I think implementing the caching in JsOptimizer and pre-warming in hook_rebuild() or similar will do exactly what we want from that point of view though.
Comment #13
mfbAs I mentioned over in #3278415: Remove usages of the JavaScript ES6 build step, the build step itself, and associated dev dependencies, seems worth exploring having a build command to do pre-minification, so that minified JS doesn't need to be part of the git repo. This build command would have to be run by the packaging script (so end users get the pre-built assets), as well as by developers and test bots working from a raw git checkout. Optionally this could also install third-party dependencies, so there would be no need to keep third-party source JS in the git repo.