Add configuration option to disable inlined CSS in SSR HTML #2067
Conversation
When inlining CSS, Angular Universal needs to extract critical styles. This seems to take up a significant chunk of processing time. However, loading may appear less smooth when this feature is disabled. Added to the configuration to make it easier to A/B test this without a full re-build.
|
@ybnd I've recently stumbled upon this as well and it seems to be a common observation that
After migrating to DSpace 7 recently I have seen very high load from the Node.js |
|
@ybnd and @artlowel : Per today's DevMtg discussion about improvements to SSR performance, it'd be good to revisit this PR and/or officially mark it "ready for review". It sounds like at least one institution (@alanorth 's) has found this useful/beneficial. So, as long as any side effects are minimal, it seems worth considering adding this to 8.0 and (likely also) 7.6.x. |
tdonohue
added
performance / caching
|
Hi @ybnd, |
|
I just wanted to add that disabling inlineCriticalCss made DSpace 7 usable in production for us. We were seeing 60+ second page loads. Thank you @alanorth for mentioning this in dspace-tech. |
|
Resolved some minor merge conflicts (due to the lint PR).
Looking back at it now, it doesn't seem that much of a problem honestly. Not sure what changed, but it looks pretty ok now. |
|
Even without this change we were seeing flashing of content |
Yes, us too. I use Firefox as my main browser as well. I assumed it was because of the web fonts via |
|
We started to use this in production as well and it is very helpful. This must be included in DSpace 8.0 please. +1 |
tdonohue
added
the
port to dspace-7_x
|
Should this not be added to |
|
@mwoodiupui if I see it correctly, this must be set in environment.ts at compile time. |
|
Then would someone please point me to the place in the code that prevents overriding this at startup, server-side. |
|
@pnbecker @mwoodiupui this should work fine from the YAML config, so probably best to include it in |
ybnd
force-pushed
the
disable-inline-css
branch
from
7aed787
to
4a39f33
Compare
|
Hi @ybnd, |
|
@ybnd : If you are able to quickly update this, I'd love to get this merged by tomorrow (Friday, April 5). That will let me deploy this on our sandbox.dspace.org site during Testathon which starts next week. Thanks! |
|
@tdonohue ready. |
|
Successfully created backport PR for |
tdonohue
removed
the
port to dspace-7_x
References
Description
The
inlineCriticalCsssetting in Angular Universal can pose performance issues.With this enabled (as it is by default since Angular 13), the server needs to determine which styles are "critical" to a given page. This can take up a good chunk of CPU time.
Turning it off would free up that time, at the expense of loading "smoothness" in the browser.
Previously I thought the decreased "smoothness" outweighed the performance impact, but especially when stacked with the "keepalive fix" the effect is pretty substantial after all.
I've also noticed that while the "flash of unstyled content" is pretty severe in Firefox, it's minimal in Chrome.
Leaving this PR as a draft for now until I can figure out how to mitigate this.
Instructions for Reviewers
Compare this PR's branch with
preboot.inlineCriticalCsson/off and see if you notice a significant difference in performanceChecklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
yarn lintyarn check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.