Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use webpack/sass-loader to precompile scss files #13204

Closed
ChristophWurst opened this issue Dec 21, 2018 · 13 comments
Closed

Use webpack/sass-loader to precompile scss files #13204

ChristophWurst opened this issue Dec 21, 2018 · 13 comments
Labels
1. to develop Accepted and waiting to be taken care of enhancement technical debt
Milestone

Comments

@ChristophWurst
Copy link
Member

Is your feature request related to a problem? Please describe.
We still compile scss files dynamically on the server, although they could be precompiled into our checked-in build artifacts (#13156).

Describe the solution you'd like
Use webpack with scss-loader (in combination with style-loader) to precompile on npm build once #13156 got merged into master.

Not only will it remove load from the server, it will make it possible to bundle more of these style sheets into a single bundle (either together with the scripts or a dedicated style bundle).

Describe alternatives you've considered
Don't change this.

cc @nextcloud/javascript @nextcloud/theming

@ChristophWurst ChristophWurst added enhancement 1. to develop Accepted and waiting to be taken care of technical debt labels Dec 21, 2018
@ChristophWurst ChristophWurst added this to the Nextcloud 16 milestone Dec 21, 2018
@rullzer
Copy link
Member

rullzer commented Dec 21, 2018

Less request => happy @rullzer

@skjnldsv
Copy link
Member

What would be the advantages?

@rullzer
Copy link
Member

rullzer commented Dec 21, 2018

@skjnldsv

  • less requests going to storage when needing to serve the file.
  • Less on the fly compilation needed on each and every nextcloud after each update

@skjnldsv
Copy link
Member

  • less requests going to storage when needing to serve the file.

Once compiled, does it change anything?

  • Less on the fly compilation needed on each and every nextcloud after each update

This one I totally get. Though you could ask how much weight does this really cost. :)

As a side note, since I've discussed it multiple times, we cannot drop the scssphp module since all of the apps still require on-the-fly compilation :)

@rullzer
Copy link
Member

rullzer commented Dec 21, 2018

  • less requests going to storage when needing to serve the file.

Once compiled, does it change anything?

Well if you have remote storage. (Like objectstorage). When a new user comes in you have to go there. Request the files and forward them.

Also it is extra requests. 1 big blob is faster than 40 little blobs.

  • Less on the fly compilation needed on each and every nextcloud after each update

This one I totally get. Though you could ask how much weight does this really cost. :)

Even for me the first load after installing or an upgrade takes significantly longer.

As a side note, since I've discussed it multiple times, we cannot drop the scssphp module since all of the apps still require on-the-fly compilation :)

Yeah that is fine. We can keep it. Apps can still use it. Though I would recommend a lot of apps to just move to the same 1 package file to server. But this is mainly about making the core what we ship faster. We can't control what any 3rdparty app out there is doing.

@skjnldsv
Copy link
Member

Though I would recommend a lot of apps to just move to the same 1 package file to server

To move to pre-compilled scss?

@rullzer
Copy link
Member

rullzer commented Dec 21, 2018

If possible yes

@skjnldsv
Copy link
Member

@rullzer that would mean any change made to server won't reflect anymore on the apps. And this goes completely away from the standardisation work I've been doing. If a fix land on 15.0.1 that change a design variable, all the apps will require a scss recompilation.

@rullzer
Copy link
Member

rullzer commented Dec 21, 2018

Well that of course should be avoided. But a lot of apps don't even use those variables. They can just bundle their css. Baby steps.

The plan is not to break anything. But a lot of things we can improve. A lot of scss doesn't even use the variables I bet

@juliusknorr
Copy link
Member

@rullzer that would mean any change made to server won't reflect anymore on the apps. And this goes completely away from the standardisation work I've been doing. If a fix land on 15.0.1 that change a design variable, all the apps will require a scss recompilation.

Are there variables we cannot just distribute via css custom properties? Because then we still can have the flexibility of predefined values from code, but can have precompiled scss.

One thing we need to check is the icon logic included for the accessiblity app. Right now, we pase all the scss files during compilation and extract icons that need to be inverted. We still would need to parse all the css files at some point to keep the accessibility app working the way it is right now, otherwise we probably need to think about a different mechanism.

@MorrisJobke
Copy link
Member

Many of them are already done, right? How much is left and should we move them then to 17?

@rullzer
Copy link
Member

rullzer commented Feb 25, 2019

Many but also many left. Yeah move to 17 so we can continue

@rullzer rullzer modified the milestones: Nextcloud 16, Nextcloud 17 Feb 25, 2019
@rullzer rullzer modified the milestones: Nextcloud 17.0.4, Nextcloud 17.0.5 Mar 11, 2020
@rullzer rullzer removed this from the Nextcloud 17.0.5 milestone Mar 23, 2020
@ChristophWurst
Copy link
Member Author

I'd say this can be closed in favor of #9940. That ticket is more or less about the same problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of enhancement technical debt
Projects
None yet
Development

No branches or pull requests

5 participants