feat: reduce size CSS output file

[ghassanmas]

This is ready again for review, see below for testing detials and result

[Update] 18.01.22 As of testing with olive MFE learning, the CSS output was missing a lot of stuff. Converted to draft again, need to double check the correct settings

The added plugin, reduce the CSS file by removing unused CSS
The plugin is kinda recommended by bootstrap¹, reducing
unused CSS is generally recommended as well enchance web
perforamce²

Related issue: openedx/wg-frontend/issues/138

This is still work on progress, I need to do further testing, to ensure this change doesn't break styling or any other thing

Footnotes

1. https://getbootstrap.com/docs/5.2/customize/optimize/#unused-css ↩

2. https://web.dev/css-web-vitals/#critical-css ↩

[openedx-webhooks]

Thanks for the pull request, @ghassanmas! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[itsjeyd]

@ghassanmas Thanks for your contribution! Please let us know when it's ready for review.

[itsjeyd]

Hey @jmbowman, this is ready for engineering review by the FED-BOM team now.

[ghassanmas]

[Update]:

Testing: I have tested this version on frontend-app-learning, by installing this version of frontend-build: see this PR openedx/frontend-app-learning#1047

The effect of this is that the css way more than expected. I don't rememeber how I test it when I opened the draft, but in any case this cannon be merged as it is. I am going convert it to draft again, and when time allow I will dig into the correct settings. s

[ghassanmas]

[Update] I think I made it to work with learning MFE olive, CSS file size was decreased 70% (few hundred kilo bytes). I had to completely set it in a different way, please check the expected diff at ghassanmas#1 . If that can of change acceptable I would tidy up and update this PR accordingly,

[ghassanmas]

The test on MFE Learning

Before This change

CSS File size: 132KB GIZ /883K/Original

Live link: https://apps.olive.zaat.dev/learning/course/course-v1:zaatdev+101+1/home

After This change

CSS File size: 27KB GIZPPED/ 278K Orginal

Live Link: https://learning.olive.zaat.dev/course/course-v1:zaatdev+101+1/home

Concolusion

Increaing lighthouse performance by 20%

[abdullahwaheed]

Its improving the build size but build time is affecting greatly with these changes. I have tested it on multiple MFEs and following are the results:
frontend-app-account:
without css optimization -> build time: 65974 ms
with optimization -> build time: 150820 ms

frontend-app-learner-portal-enterprise:
without css optimization -> build time: 73184 ms
with optimization -> build time: 268651 ms

frontend-app-learning:
without css optimization -> build time: 121364 ms
with optimization -> build time: 239722 ms

@arbrandes what are your thoughts on this?

[ghassanmas]

Yes I expected that would occuar, but for me I think the priority comes first for end user which is leaner.

For example at 2U/edx you do one build for milllions of users, compare those few seconds with a fraction of second of for million of users.

Edit: Digging into numbers the channge seems reasonable for learning, but for others it's way too much, also it could you please set how you did the build, with optimizizaiton was there somehting working on computer while building? were you building concurrently....etc

[Edit] on reapting test on the account MFE, the average increase time is ~30%

[arbrandes]

I have to be honest: I don't much like CSS removers. There's too much risk for too little reward. This article outlines some of the dangers:

https://css-tricks.com/how-do-you-remove-unused-css-from-a-site/#aa-part-of-a-build-process

The added build time is also bad. We're trying to reduce it for many reasons, and this will make it much worse. I don't think saving 100KB of downloads is worth it.

What I'd be more interested in is ways to avoid having to insert framework CSS when it's not necessary in the first place, possibly by picking more modular frameworks.

[itsjeyd]

@ghassanmas A friendly reminder to follow up on @arbrandes's comment above.

[ghassanmas]

@itsjeyd we have discussed this in the last FWG, https://openedx.atlassian.net/wiki/spaces/COMM/pages/3645964289/2023-01-26+Frontend+Working+Group+Meeting+Minutes. And I think @arbrandes want to check with folks opinion before merging it.

[ghassanmas]

This is again ready for review:

• It's now optional to use prugecss, readme.rst has been updated by default it's not used
• It include ts and tsx file checks

[ghassanmas]

@arbrandes following your suggestion I made it optional, and as for tutor-mfe I geuss it worth mentioning this option, and I would suggest that the default tutor-mfe image to use it since it would be build once and used many times.

[itsjeyd]

label: core contributor

[abdullahwaheed]

LGTM

[arbrandes]

Looks good in principle, but I think the code could still benefit from a couple of changes.

Thanks for bearing with me, @ghassanmas.

[ghassanmas]

@arbrandes no worries, I did your suggestion and have rebased to single commit. Have tested again:

• USE_PURGECSS=true npm run build Would enable it reduces app.*.css file by size and increase build time
• npm run build nothing changes

Testing while applying the following patch in account MFE:

-    "@edx/frontend-build": "9.1.1",
+    "@edx/frontend-build": "git+http://github.com/ghassanmas/frontend-build.git#6680a856779fd8a204ef290af3d1f658e4e3f3e6",
     "@edx/reactifex": "^1.0.3",

[itsjeyd]

@ghassanmas Looks like this is getting closer to being merged :)

Could you please resolve merge conflicts and ping @arbrandes for another round of review when done?

[ghassanmas]

@itsjeyd done from 32-155 :)

[itsjeyd]

Thanks @ghassanmas!

@arbrandes Back to you :)

[arbrandes]

Looks great! As usual, thank you for you patience, @ghassanmas, and congrats on pushing this to completion.

[openedx-webhooks]

@ghassanmas 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-semantic-release]

🎉 This PR is included in version 12.8.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀