[4.0][RFC] Drop minified static files from the repo

[dgrammatiko]

Pull Request for Issue # .

Summary of Changes

• All minified css and js files were removed from the repo
• JHTML (or better HTMLHelper) was adjusted to pick the right files depending on debug state and irrelative to the given extension (eg .min.js or .js)

Why

• This will make contributors life easier as they won't have to mess with node and try to figure out how to minify js.
• It's kinda unsafe to accept minified code...

Possible problems

• The repo remains installable but doesn't contain the production preferred minified files, so someone downloading it from GitHub will have to run build in order to get to production mode.
• Although the idea here is to help contributors by offering an easer path for involvement and there is some security issue by accepting minified code, we do increase our dependency on tools and maybe this requires some more testing. Probably the release team can shed some light here.

WIP

As I opened the PR for comments there are parts that are missing here:

• For SASS we need to see if we can trigger the minifier per request, so things could be tested.
• The build script needs to run the node uglifier then pack the packages and then remove the minified files again. This code is not included in the PR (it makes no sense to spent time if this won't be approved)

Testing Instructions

Apply patch, or better download the branch from issue tracker and install. Check debug options and that all css and js files are still loaded correctly

Expected result

Actual result

Documentation Changes Required

Yes but mainly this will affect the maintainers (for the rest contributors is just removing the requirement for compressing js)

Calling @mbabker @wilsonge @rdeutz @dneukirchen @yvesh

[dneukirchen]

I like it.
👍 Makes dev life a lot easier.
👍 Nobody really reviews whats committed in minified code.
👍 Those who want to install or deploy from github to production should be able to run the build by themselves.
😿 Will brake some tools...

[mbabker]

The repo is not installable as is anymore, so someone downloading it from GitHub will have to run build before actually installing it.

That's the problem. The repo has always been in an installable state and if this is going to be accepted then we need to remove libraries/vendor from VCS too.

Likewise, before this can be merged whatever updates are needed so that distributable packages can be built (i.e. nightly builds, and eventually the production packages) must be included.

Documentation Changes Required

More than just affecting maintainer workflow. You are in effect changing workflow for all contributors because the acceptance of this PR means one can no longer simply git clone the repo and go to work, they must be willing to go through additional steps to get to a usable state.

This also puts a lot of trust in the build process because this means there are going to be a lot of files included in packages that will have zero opportunity for test/review until someone creates a package. For in house framework built apps not checking compiled assets into the repo is fine because of the workflows we use, and it not being mass distributed code, but I worry a bit about opening the door to us distributing possibly hundreds of files that have no chance for test/review to hundreds of thousands of sites.

[dgrammatiko]

The repo has always been in an installable

Actually the repo remains installable and works fine as is, but for production use will not be a good fit as all the js is not minified. Sorry didn't phrase that correctly! :(

distributing possibly hundreds of files that have no chance for test/review

Pretty sure we can set some safety net, so this whole procedure is not going down to trusting blindly someone else's uglifier code.

[brianteeman]

I am not in favour of any PR that means there are files in the release which have not been extensively tested

[dgrammatiko]

@brianteeman that will not be the case, since we have https://github.com/joomla-projects/gsoc17_pr_testing_platform where we can add the minified scripts (eg run node uglify)

[dneukirchen]

I am not in favour of any PR that means there are files in the release which have not been extensively tested

Got the point, but the problem is that at the moment, nobody (no person or tool) tests/reviews those files (expecially the minified scripts). Thats a possible security risk. Someone could commit malicious code (i.e. a credit-card or password scraper) in the minified version of the script without getting noticed.

We need to improve CI, automated testing and workflows, but im still in favor of this and think it is the right step.

[brianteeman]

Not saying the custom system is perfect but i dont agree with this method of improvement

@dgt41 if that ever gets implemented I would be happy to reconsider my opinion

[mbabker]

Someone could commit malicious code (i.e. a credit-card or password scraper) in the minified version of the script without getting noticed.

Then we need to improve our tooling to validate these files, not trust that myself, George, and Robert's personal systems as well as the CI infrastructure we use to build nightly packages are able to build release packages and never be compromised. I'm comfortable shipping non-version controlled files in projects where I have full control over the source and deploy workflow, but personally I'm less willing to push my luck right now on a mass distributed project like Joomla, it just seems far too risky at the moment.

[dgrammatiko]

@mbabker @brianteeman but we still have the release team that tests things before the hit the public, or not?
The only difference here is who initiates the node uglifyjs and so far (all these months we're using this) we never had a glitch in this procedure (I mean getting an uglified file messed up). I'm not saying that this might not happen but there are multiple level of protection here:

• Lock the uglify code to specific version,
• Update it only after chorally tested that the results of all files are the same as the previous version
  ...

It's not black or white, but this is a step forward

[laoneo]

I think it would be a step into the right direction. The advantages do overweight the concerns. Going that path would also simplify the dev workflow of the media manager.

then we need to remove libraries/vendor from VCS too

Agree here.

An issue I see is with our human patch testing. When they install a nightly and do not turn on the debug mode, then they are not testing the js changes in a patch. Probably we need to add an alert in the patch tester too if debug mode is disabled.

[mbabker]

Patch tester becomes unusable once we remove assets from the repository without requiring Node and Composer.

[dgrammatiko]

then they are not testing the js changes in a patch

But we can adjust the nightly to run node uglify before packing (or not, haven’t checked that code)

[mbabker]

But we can adjust the nightly to run node uglify before packing (or not, haven’t checked that code)

It would have to be adjusted. Nightly builds are basically the same as the full release builds, just a hacked script to be able to build from branch versus tag and only build ZIP packages.

[brianteeman]

@mbabker @brianteeman but we still have the release team that tests things before the hit the public, or not?

Your guess is as good as mine https://volunteers.joomla.org/teams/cms-release-team

[brianteeman]

An issue I see is with our human patch testing. When they install a nightly and do not turn on the debug mode, then they are not testing the js changes in a patch.

Not true

[C-Lodder]

Agree with removing minified files from the repo for both CSS and JS.

Should be up to the CMS maintainer to run the build scripts before publishing a release on Github.

This does indeed make life mich easier for those who contribute code, more so those who have never heard of Node.

Will also mean less conflicts.

[brianteeman]

TBH there are a lot more important issues to address with j4 than this PR - there really has been almost no significant movement since the alpha release

[dgrammatiko]

@brianteeman that’s why we try to address what seems to be unimportant issues since there are no major tasks on the table :(

[brianteeman]

There are lots of major tasks but they all seem to have stalled :(
By the time 4 is released web components will be last years trend

[C-Lodder]

Being part of a small group who manages CSS ans JS, can I also say I'm verging on sick to death of dealing with conflicts, most of which come from minified files

[mbabker]

There are lots of major tasks but they all seem to have stalled :(

I can keep doing code stuff but when there are 3 people who do all the architecture stuff I don't have the desire to move very quickly on that (I'm doing PHPCS upgrades on 80 branches of our code, a more productive use of my time, believe it or not).

Anything UI related personally I'm not touching (including install from web) until the next template is produced.

So that leaves working on tool chains. Which is what this PR does. I get all the reasoning behind it, personally I have no issue with the approach, but fundamentally it's a major shift requiring a lot of buy in from a lot of sources.

[Fedik]

The repo is not installable as is anymore, so someone downloading it from GitHub will have to run build before actually installing it.

no, it is not that big problem, if we do the thing right (see bottom) 😉

then we need to remove libraries/vendor from VCS too

no, it is different,
we still have all JS/CSS/SCSS sources under VCS, we just remove minified versions

What about compromise?

We remove all minified versions from repo as @dgt41 sugested.
But.
We keep pre-compiled SCSS.
We keep all sources in the place where they should be in the production.
And make HTMLHelper to always use source (files without .min.blabla) when $version->isInDevelopmentState() === true.

So the repo stays installable and usable and testable.
A testers will always test a source, that will avoid a problem when source and minified version are different, that happens time to time with different scripts.
We still safe from security point of view, because all minified versions will be compiled from sources which is in our repo.

in the theory all should work good 😄

[dgrammatiko]

And make HTMLHelper to always use source (files without .min.blabla) when $version->isInDevelopmentState() === true.

@Fedik this is already implemented here in the HTMLHelper : https://github.com/joomla/joomla-cms/pull/19637/files#diff-8f6485b51d191985c4d1e9fbbf1a743aR1311

[dgrammatiko]

This RFC served it's purpose so longer needs to be open, lets move the discussion to #19744