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...

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.

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.

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

@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)

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.

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

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.

@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

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.

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

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)

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.

@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

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

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.

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

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

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

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

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.

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 ?

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

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