Ok, so I just force-pushed with some updates:

• Actually writes out files to disk now.
• Source map support.
• Turn down logging unless DEBUG is set in the environment.
• Fixed lint complaints about console.log.

And I checked the errors caused by files that contain non-top-level imports; those really are the only kind of error. Tried it on other modules with some success.

So now I am going to look at doing some minification of JS inside JSP — that might be rather tricky though because the technique we use to run Prettier on JS inside JSP may not transfer across so well (because it is pretty whitespace-sensitive), but I'll investigate and report back what I find here.

Added a (WIP) commit that formats JS in JSP. You can see this in the output of accounts-admin-web, were a file like src/main/resources/META-INF/resources/account_users_admin/select_account_entry.jsp gets mangled to build/node/packageRunBuild/resources/account_users_admin/select_account_entry.jsp (edit: that path isn't quite right, but you can still see what the contents look like if you click on the link).

Here's the diff; it actually doesn't look too shabby at all, but will need some tweaking to get it totally right.

Notably not handled by this approach are sneaky JSP files that contain JS outside of <script> (or <aui:script>) tags. For example:

• frontend-js-web/src/main/resources/META-INF/resources/liferay/available_languages.jsp
• Which is exposed sneakily here, as far as I can tell.

I'll take this up again tomorrow to see if I can polish it up enough to mark this as ready for review.

Hey @wincent, I think this makes sense!

Just to clarify, we're always producing foo.js -> foo.js regardless of wether we're minifying or not, correct? Can't spot a specific setting to rename the files, but just want to make sure it's not a default.

I'd say we can park JSP minification unless you think it's very low-hanging fruit and then figure out whether it's worth it or not.

Do you think we have enough already in the analysis phase to wrap it up and start planning this for real? (It feels like we're already well past that 😂 )

Just to clarify, we're always producing foo.js -> foo.js regardless of wether we're minifying or not, correct? Can't spot a specific setting to rename the files, but just want to make sure it's not a default.

We overwrite all existing .js files under build in place, except ones which already have the -min.js or .min.js suffixes (this matches the heuristic used on the server at the moment). That's what makes this change so minimal, and means it handles our own JS source, source from node_modules that has gone through arbitrary transformation by the bundler (v2 or v3), or through webpack.

I'd say we can park JSP minification unless you think it's very low-hanging fruit and then figure out whether it's worth it or not.

So I added another commit polishing that up. I'd say it's pretty "solid" at this point. I see one sort-keys lint failure so I am going to fix that up and push a commit for that too.

Do you think we have enough already in the analysis phase to wrap it up and start planning this for real? (It feels like we're already well past that 😂 )

Indeed we are, but the thing with abstract stuff like this is that it is much easier to discuss when you have a concrete artifact. Questions like "is it really going to be that much simpler to do a single build rather than a dual build", or "how many bytes is this actually going to save us", or "is it worth the effort of minifying code in JSP" all become trivially easy to answer at this point. To that effect I think on Monday I am going to run this over all of DXP to get some before-and-after numbers, and I'll do some quick smoke testing to show that DXP has a chance of running properly after a fully minified ant all.

And after that I don't know what planning is left to do.

And after that I don't know what planning is left to do.

Exactly 😂

[...] the thing with abstract stuff like this is that it is much easier to discuss when you have a concrete artifact

Yeah, I think we're done with analysis the moment we make an educated guess as to which direction we wanted to push for and work on this in npm-scripts rather than doing a quick and dirty test on DXP.

If (as many times happens) our assumptions are proven wrong, then we simply step back and approach the problem differently.

Cut a v34.1.0-pre.0 in order to test this and gather some metrics after a full ant all in liferay-portal (created LPS-123515 for that). I expect some things to break because the default minification is likely a bit more aggressive than what we get with Google's Closure compiler running in "simple" mode, so we may have to tweak some settings to dumb it down a bit.

Was going to rebase this on master because of the merge conflicts, but it seems the yarn.lock is bogus and should be fixed separately (ie. running yarn on master creates a large diff, and I think we might have a problem with our tags — one of the deps it adds is @liferay/[email protected], but we only have a tag for @liferay/[email protected]).

So I'll just push what I have over this stale history; this change kind of blows my mind:

chore(npm-scripts): make minification more conservative

Based on testing across the liferay-portal repo, the property mangling needs to be turned off because it does some IMO surprising things. For example, in:

https://github.com/liferay/liferay-portal/blob/c74cbb3487428c54ae2122c205af3c954e0e01a3/modules/apps/frontend-js/frontend-js-aui-web/src/main/resources/META-INF/resources/liferay/modules.js#L18-L21

This:

(function () {
   var LiferayAUI = Liferay.AUI;

   var COMBINE = LiferayAUI.getCombine();

gets mangled to:

!function(){var e = Liferay.AUI,a=e.i()

ie. global Liferay.AUI gets preserved (good), but a call to its getCombine() method gets unfathomably rewritten to i(). No idea why it would think it's legit to do that, but whatever, we can turn off that kind of mangling with the following effects:

• With unwanted mangling (ie. prior to this commit):

  Before size:       616616
  After size:        201246
  Delta:             415370
  

• With mangle: false (ie. no mangling at all):

  Before size:       616616
  After size:        353013
  Delta:             263603
  

• With mangle: {properties: false} (ie. this commit):

  Before size:       616616
  After size:        266414
  Delta:             350202
  

So we lose a bit of minification but not all of it (ie. instead of trimming 405K, we trim 341K, which is not quite as bad as if we'd turned off all mangling, which would mean trimming only 257K).

The other step in a conservative direction that I'm taking in this commit is turning off some other renames that might cause problems when minifying JSP. I haven't seen any evidence that this is needed yet, but am erring on the side of caution.

Resolved merge conflicts and force-pushed; no other changes.

Are we going with minified JS in JSP, then?

Are we going with minified JS in JSP, then?

I'm testing in DXP again today. If it goes poorly, we can rip out the minifying-in-JSP to expedite this. If it goes well, we may as well use it.

I'm testing in DXP again today. If it goes poorly, we can rip out the minifying-in-JSP to expedite this. If it goes well, we may as well use it.

Here's to it going well!! 🥂

Ok, summarizing my testing.

I think this breaks little or nothing — screenshot shows a minified Clay file, but similar results for other node modules, and DXP-internal modules:

Also all the tests are green over on my fork:

✔️ ci:test:stable - 9 out of 9 jobs passed

✔️ ci:test:relevant - 20 out of 20 jobs passed in 2 hours 54 minutes

But some things are escaping minification; I'll look into these to see if I can discover a pattern:

And source maps don't seem to work. They are detected, but I don't see the original source:

Finally — minification in JSP didn't happen, because of ordering and/or caching issues. While it worked previously in my per-module test deployments, it doesn't seem to have taken effect globally during ant all:

That's corresponds to these lines from product-navigation-site-administration, which I naïvely expected to have been minified — but when I look in there, I see that there is no package.json in that module, so liferay-npm-scripts doesn't get the chance to do its thing there.

More concerning though, is that it's not working for modules like this file in account-admin-web, which doesn have a package.json that calls liferay-npm-scripts build.

If I run yarn run build in that module, it does work; ie. JS gets minified to:

<aui:script use="liferay-search-container">var searchContainer=Liferay.SearchContainer.get("<portlet:namespace />accountUsers");searchContainer.on("rowToggled",(function(event){var selectedItems=event.elements.allSelectedElements,result={};selectedItems.isEmpty()||(result={data:{value:selectedItems.get("value").join(",")}}),Liferay.Util.getOpener().Liferay.fire("<%= HtmlUtil.escapeJS(eventName) %>",result)})),Liferay.Util.selectEntityHandler("#<portlet:namespace />selectAccountUser","<%= HtmlUtil.escapeJS(eventName) %>");</aui:script>

and I can deploy that and see it working (ie. see the console.log in the minified source):

• ant all = no minification

• yarn run build = minification works

• gradlew clean deploy -a = no minification

• gradlew deploy -a = gradlew skips yarn build

  Task :apps:account:account-admin-web:packageRunBuild UP-TO-DATE

• gradlew clean deploy -a = gradlew skips yarn build again

  Task :apps:account:account-admin-web:packageRunBuild FROM-CACHE

• yarn run build && gradlew deploy -a = gradlew does yarn build again, and minification works

  Task :apps:account:account-admin-web:packageRunBuild
  $ liferay-npm-scripts build
  Minification summary:
  Files processed: 73
  Files with errors: 0
  Before size: 193469
  After size: 171197
  Delta: 22272
  Elapsed: 0.63

  but inspecting the JAR at build/tmp/jar/com.liferay.account.admin.web-2.0.0.jar shows it to contain unminified JS.

So I'll keep poking around a bit to see if I can get the JSP thing working, but I suspect that the fiendish devilry of Gradle's dependency analysis and caching means that it is probably going to fall into the "not worth it category".

But some things are escaping minification; I'll look into these to see if I can discover a pattern:

I think the AUI ones are expected since they're probably simply moved over in their final form from either a maven artefact or an npm dependency into to the build without passing through src and probably skipping the whole build process. I'd expect other dependencies to go to through a similar process.

More concerning though, is that it's not working for modules like this file in account-admin-web, which doesn have a package.json that calls liferay-npm-scripts build.

I think that's also what @bryceosterhaus saw in the analysis for LPS-123287 - Analyze Sass compilation pipeline

Other modules that use BuildCSS but not npm-scripts, migrate those to use npm-scripts.

FWIW I asked the build wizards over in #build about this, but is mostly a theoretical exercise at this point. My testing shows that the tasks run in this order, basically:

gradlew compileJava -a # puts 12 directories, 1 file in build/, and 35 directories, 97 files into classes/ — no JSP yet
gradlew packageRunBuild -a # would minify JSP, but it's not there yet
gradlew processResources -a # puts 54 directories, 158 files in build/, 61 directories, 256 files into classes/ - JSP is now in there
gradlew classes -a # doesn't do anything: > Task :apps:account:account-admin-web:classes UP-TO-DATE

Full text of my question for posterity:

i'm trying to figure out if there's a nice way to insert some additional processing into the JSP build process, and I'm wondering if somebody wise in the ways of our gradle tasks can confirm my understanding of how it works

specifically, the high-level goal is to extract JS from <script> (and <aui:script>) tags in JSP files, pre-minify it, and inject it back in. i have a proof of concept that does that in a hacky way during the packageRunBuild task by overwriting JSP files under classes/

now, this works as a demo but not in practice because the actual order the tasks run is something like this:

compileJava - Compiles main Java source.
packageRunBuild - Runs the "build" package.json script.
processResources - Processes main resources.
classes - Assembles main classes.

ie. compileJava sticks a few bits and pieces in build/ and classes/, but doesn't put the JSP in there yet, then runs packageRunBuild, which would do JS-in-JSP pre-minification if there were any JSP files in classes/ (but there aren't yet), then runs processResources which is what actually shoves the JSP in there

so I guess my questions are these:

1. is there a better (non-hacky) way to "pre-process" or "post-process" the JSP files, ideally something we could do from packageRunBuild without introducing another task
2. failing that, would it be feasible to change the dependency relationship so that packageRunBuild runs after processResources (ie. when the JSP files are there to operate on)
3. and failing that, if we were to add another task as a post-processing step, how difficult is that to do and would the "juice be worth the squeeze"? so to speak