Skip to content

Remove SCSSCacher and compile scss files with npm-sass #32261

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.

Sign up for GitHub

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

Jump to bottom
Merged
skjnldsv merged 8 commits into from
May 13, 2022
Merged

Remove SCSSCacher and compile scss files with npm-sass #32261

skjnldsv merged 8 commits into from
May 13, 2022

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented May 4, 2022

Replace #31743

@skjnldsv skjnldsv added this to the Nextcloud 25 milestone May 4, 2022
@skjnldsv skjnldsv self-assigned this May 4, 2022
CarlSchwan
CarlSchwan reviewed May 4, 2022
View reviewed changes
@CarlSchwan CarlSchwan mentioned this pull request May 4, 2022
Closed
2 tasks
@CarlSchwan
Copy link
Member

CarlSchwan commented May 5, 2022 •
edited by skjnldsv
Loading

To give a small performance overview in term of db requests

loading dashboard page:

  • before: 19
  • after: 12

loading photo page:

  • before: 34
  • after: 22

loading files page:

  • before: 31
  • after: 23

This is with a hot redis cache. With a cold cache before the numbers were always above 100 and after this patch: the number is almost the same with and without hot cache. So really nice improvement :)

artonge
artonge reviewed May 5, 2022
View reviewed changes
@PVince81
Copy link
Member

PVince81 commented May 5, 2022

@kesselb
Copy link
Contributor

kesselb commented May 6, 2022

cc @MorrisJobke as we talked about removing the scss cacher a while ago 😎

@skjnldsv skjnldsv mentioned this pull request May 10, 2022
Merged
5 tasks
@skjnldsv skjnldsv force-pushed the feat/remove-scss-support branch 2 times, most recently from 702d420 to cbbf192 Compare May 11, 2022 09:04
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 11, 2022
@skjnldsv skjnldsv requested review from a team, artonge, come-nc, nickvergessen, szaimen and vanpertsch and removed request for a team May 11, 2022 09:44
@skjnldsv skjnldsv force-pushed the feat/remove-scss-support branch from 0960be6 to ce39e3f Compare May 11, 2022 09:45
@skjnldsv skjnldsv requested a review from CarlSchwan May 11, 2022 15:11
@PVince81
Copy link
Member

PVince81 commented May 11, 2022

cool!

so as I understand now there will be no more CSS or icon files in the appdata directory ?
if that's correct, do we need a migration routine to clean up those folders also ?

@skjnldsv
Copy link
Member Author

skjnldsv commented May 11, 2022

so as I understand now there will be no more CSS or icon files in the appdata directory ? if that's correct, do we need a migration routine to clean up those folders also ?

That is correct.
Good point, will do as a followup :)

@skjnldsv skjnldsv force-pushed the feat/remove-scss-support branch from ce39e3f to f4a6d3b Compare May 12, 2022 07:54
@PVince81

This comment was marked as resolved.

Sign in to view
@skjnldsv skjnldsv requested a review from PVince81 May 12, 2022 09:23
@skjnldsv
Copy link
Member Author

skjnldsv commented May 12, 2022

Addressed @PVince81 :)
Also found some other fixups from Vanessa 🚀

@skjnldsv skjnldsv force-pushed the feat/remove-scss-support branch from 641e007 to 55534d5 Compare May 12, 2022 11:01
@nickvergessen
Copy link
Member

nickvergessen commented May 13, 2022

Conflicting files
core/css/toast.scss
dist/workflowengine-workflowengine.js
dist/workflowengine-workflowengine.js.map

PVince81
PVince81 approved these changes May 13, 2022
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks fine now

next up: adjust all apps we know of

@CarlSchwan
Copy link
Member

CarlSchwan commented May 13, 2022

Tested again and works fine here :)

@skjnldsv skjnldsv mentioned this pull request May 13, 2022
Closed
skjnldsv added 8 commits May 13, 2022 16:10
@skjnldsv
Remove SCSSCacher
9a9409e 
Signed-off-by: John Molakvoæ <[email protected]>
@skjnldsv
Migrate core scss
cc6037e 
Signed-off-by: John Molakvoæ <[email protected]>
@skjnldsv
Migrate apps scss
78b7bae 
Signed-off-by: John Molakvoæ <[email protected]>
@skjnldsv
Npm script
3e29e0a 
Signed-off-by: John Molakvoæ <[email protected]>
@skjnldsv
Cleanup and compile
73918b8 
Signed-off-by: John Molakvoæ <[email protected]>
@skjnldsv
Add workflow test
29a8b29 
Signed-off-by: John Molakvoæ <[email protected]>
@skjnldsv
Fix jsunit
b8a7ab9 
Signed-off-by: John Molakvoæ <[email protected]>
@skjnldsv
Fix profile and add gradient-primary-background
264adf0 
Signed-off-by: John Molakvoæ <[email protected]>
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 13, 2022
@skjnldsv skjnldsv force-pushed the feat/remove-scss-support branch from 55534d5 to 264adf0 Compare May 13, 2022 14:13
@skjnldsv
Copy link
Member Author

skjnldsv commented May 13, 2022

Rebased, ready to get in

nickvergessen
nickvergessen approved these changes May 13, 2022
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally looked at the full diff and made sure it works in every corner case like it should

@skjnldsv skjnldsv merged commit aff3740 into master May 13, 2022
@skjnldsv skjnldsv deleted the feat/remove-scss-support branch May 13, 2022 15:28
@szaimen
Copy link
Contributor

szaimen commented May 13, 2022

🎉🎉🎉🎉🎉

@MorrisJobke
Copy link
Member

MorrisJobke commented May 16, 2022

Nice @skjnldsv! Thanks for moving this one forward 🥳

@nickvergessen nickvergessen mentioned this pull request May 16, 2022
Closed
9 tasks
@Pytal Pytal mentioned this pull request Jun 10, 2022
Closed
@ArtificialOwl ArtificialOwl mentioned this pull request Nov 15, 2022
Merged
@ChristophWurst ChristophWurst mentioned this pull request Dec 5, 2022
Merged
5 tasks
@joshtrichards joshtrichards mentioned this pull request Nov 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@PVince81 PVince81 PVince81 approved these changes

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@szaimen szaimen Awaiting requested review from szaimen

@come-nc come-nc Awaiting requested review from come-nc

@artonge artonge Awaiting requested review from artonge

@vanpertsch vanpertsch Awaiting requested review from vanpertsch

Assignees

@skjnldsv skjnldsv

Labels

4. to release Ready to be released and/or waiting for tests to finish feature: theming performance 🚀 technical debt

Projects

None yet

Milestone

Nextcloud 25

Development

Successfully merging this pull request may close these issues.

9 participants

@skjnldsv @CarlSchwan @PVince81 @kesselb @nickvergessen @szaimen @MorrisJobke @artonge