Server: Combine sass files

[ChristophWurst]

We need to get the number of requests down a bit as discussed in #2272. Therefore, I've started a PR to create an image sprite for the icons (#3385). Furthermore, I plan to combine the vendor scripts we load on all pages, like jQuery, it's plugins, Backbone and the like.

We do also load a bunch of style sheets, of which some are included on all pages as well. Therefore, I've been thinking about saving some requests there too. As discussed with @skjnldsv on IRC, the simplest solution from my point of view, would be to create some kind of meta files, where we import all the source files that we load individually now.

This would looks like this:

 core/css/server.scss            | 31 +++++++++++++++++++++++++++++++
 lib/private/legacy/template.php | 18 ++++++------------
 2 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/core/css/server.scss b/core/css/server.scss
new file mode 100644
index 0000000000..2048a881dd
--- /dev/null
+++ b/core/css/server.scss
@@ -0,0 +1,31 @@
+/**
+ * @author Christoph Wurst <[email protected]>
+ * @copyright Christoph Wurst <[email protected]>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+@import 'styles.scss';
+@import 'inputs.scss';
+@import 'header.scss';
+@import 'icons.scss';
+@import 'fonts.css';
+@import 'apps.scss';
+@import 'apps.scss';
+@import 'global.css';
+@import 'multiselect.scss';
+@import 'tooltip.scss';
diff --git a/lib/private/legacy/template.php b/lib/private/legacy/template.php
index a07bf214f3..07dbe7d6a6 100644
--- a/lib/private/legacy/template.php
+++ b/lib/private/legacy/template.php
@@ -106,19 +106,13 @@ class OC_Template extends \OC\Template\Base {
                }
            }

-           OC_Util::addStyle("tooltip",null,true);
-           OC_Util::addStyle('jquery-ui-fixes',null,true);
+           OC_Util::addStyle('jquery-ui-fixes', null, true);
+           OC_Util::addStyle('mobile', null, true);
+           OC_Util::addStyle('fixes', null, true);
+           OC_Util::addStyle('global', null, true);
+           OC_Util::addStyle('server', null, true);
+
            OC_Util::addVendorStyle('jquery-ui/themes/base/jquery-ui',null,true);
-           OC_Util::addStyle("mobile",null,true);
-           OC_Util::addStyle("multiselect",null,true);
-           OC_Util::addStyle("fixes",null,true);
-           OC_Util::addStyle("global",null,true);
-           OC_Util::addStyle("apps",null,true);
-           OC_Util::addStyle("fonts",null,true);
-           OC_Util::addStyle("icons",null,true);
-           OC_Util::addStyle("header",null,true);
-           OC_Util::addStyle("inputs");
-           OC_Util::addStyle("styles",null,true);

            // avatars
            if (\OC::$server->getSystemConfig()->getValue('enable_avatars', true) === true) {

The result: on my dev instance 173 instead of 185 were necessary to load the files app with cleared cache (ctrl+f5). Note that this does only affect the server files. We could do the same with files, sharing, settings and so on and save even more requests.

@nextcloud/designers would do you think? Any objections? I know @skjnldsv doesn't like the approach 😀

[skjnldsv]

Why not include mobile, global, fixes... ?

Just as a comparison, could you provide before/after benchmarks? :D

[ChristophWurst]

Why not include mobile, global, fixes... ?

I wasn't sure if that's okay because those are css and not scss files. Can I just import them in the same fashion or should we rename them before?

Just as a comparison, could you provide before/after benchmarks? :D

As stated above, 173 instead of 185 requests 🚀

[skjnldsv]

siege -r6 -c10 -d10 --header="Authorization:Basic YWRtaW46YWRtaW4=" https://localhost/index.php/apps/files/

Without concatenation:

Transactions:               9240 hits
Availability:             100.00 %
Elapsed time:              60.65 secs
Data transferred:         192.46 MB
Response time:              0.02 secs
Transaction rate:         152.35 trans/sec
Throughput:             3.17 MB/sec
Concurrency:                3.16
Successful transactions:        9240
Failed transactions:               0
Longest transaction:            0.64
Shortest transaction:           0.00

With concatenation:

Transactions:               8760 hits
Availability:             100.00 %
Elapsed time:              51.89 secs
Data transferred:         191.36 MB
Response time:              0.01 secs
Transaction rate:         168.82 trans/sec
Throughput:             3.69 MB/sec
Concurrency:                2.28
Successful transactions:        8760
Failed transactions:               0
Longest transaction:            0.83
Shortest transaction:           0.00

[skjnldsv]

@ChristophWurst importing css files should be ok. The compiler should ignore it.

We need to be sure this process isn't too heavy for the server. And another issue, since the algorithm is relying on the mdate of the scss file to check if the cached css needs a recompilation or not won't work anymore if we use one single scss file 😞

[MorrisJobke]

We need to be sure this process isn't too heavy for the server. And another issue, since the algorithm is relying on the mdate of the scss file to check if the cached css needs a recompilation or not won't work anymore if we use one single scss file 😞

So we need to touch this file for every change in the imported css files?! 😕

[skjnldsv]

It won't work. @import doesn't care about the other files. Only server.scss will be used as a comparison

[go2sh]

Why would it be so bad to do the scss compilation as a build step? From a user point of view nothing changes. And if someone wants to run a dev (git) Version, screw the number of requests.

[skjnldsv]

Because we don't want to increase the level entry barrier. This has been discussed long time before launching the scss integration :)

[go2sh]

Hmm, I see. I just followed the discussion around this topic and spent some thoughts on it. It is quite hard to create a modern fully featured webapp, which nextcloud has become, with a low entry barrier. The hole asset workflow is very unique and the code is so complex and/or split, that npm install and npm run build (or what ever) doesn't add any complexity from my point of view...

But in the end it all comes down to this: Who wants to be modular, gets a lot of modules.

[juliusstoerrle]

I'm not sure if I understood the issue correctly, but can't we implement a button in the admin menu to recompile the full css?

[MorrisJobke]

I'm not sure if I understood the issue correctly, but can't we implement a button in the admin menu to recompile the full css?

No. we should do this completely in the background. The problem is also not about compiling it, but about concatenate the correct ones together in the correct order and properly detect that a change happened. No admin wants to do this manually and it is super error prone.

[webermax]

@ChristophWurst could you explain "some kind of meta files", please?

Having smooth CSS/JS loading in the background without adding a build process would be the best solution indeed and separating SCSS functionality would be mandatory then.

Would it be an idea to implement "styles.php" and "scripts.php" for concatenating, minifying and caching nextcloud assets?

[jancborchardt]

What are the arguments against it @skjnldsv?

And @go2sh

npm install and npm run build (or what ever) doesn't add any complexity from my point of view...

Currently you don't need to do anything. Then you would need node. Of course it adds complexity.
Please don't assume everyone is familiar with this. Especially designers and other contributors which we need have issues with this.

[skjnldsv]

@jancborchardt no arguments against it, the sass combination is a good idea. But we need to be sure that a change on any scss file triggers a recompilation. If we go for a server.scss file, only changes made to this file will trigger a recompilation :)

[go2sh]

@jancborchardt Hmm ok. But anyway, it would be possible to distinguish between development and release. You could implement a development mode, where you can use the files as they are and for a release the files could be combined.

[MorrisJobke]

@jancborchardt Hmm ok. But anyway, it would be possible to distinguish between development and release. You could implement a development mode, where you can use the files as they are and for a release the files could be combined.

But then we need to verify it is actually triggered during release upgrades and cause weird side effects if one code path is used for dev and for release we use a completely different one. I'm more in favour of a solution that maybe checks the imports for a file and then also checks for the mtime of all imported files. Is this possible @skjnldsv? Or would that mean we need to keep track of too much? This info can also be easily cached, because we only need to update the list of includes once we notice a mtime change on the file containing the @import statements.

[rullzer]

So it should not be 'hard' to keep track of a list of dependencies. However it will be a performance hit when we have to look it up. And you get into what if there is another dependency etc which is pretty error prone.

A build script would certainly make a lot of stuff a whole lot easier! (yeah yeah I know the policy).

Having said that all if we forbid nested includes. Then this should be doable.

[rullzer]

Oooo our compiler even has a very fancy feature for this! Let me see what I can whip up!

[MorrisJobke]

Fix is in #3791