[AssetMapper] Automatically preload CSS files if WebLink available #51829
Conversation
|
The only problem would be: Google Pagespeed and other performances tests/tools rate really bad if you preload a CSS you do not use "quickly" https://web.dev/preload-critical-assets/#how-preloading-works |
But if we KNOW that what we are preloading WILL be |
I'll really need to try it "in the wild" (ux.symfony.com ? 👼) to be 100% sure... but i'd say "yes" :) And anyone so obsessed with performances still can call manually the stylesheets so i'm not worried :) |
weaverryan
added
the
❄️ Feature Freeze
weaverryan
force-pushed
the
asset-mapper-css-preload-weblink
branch
from
edcc1e9
to
6a9d1ff
Compare
| @@ -27,6 +33,7 @@ public function __construct( | |||
| private readonly string $charset = 'UTF-8', | |||
| private readonly string|false $polyfillUrl = ImportMapManager::POLYFILL_URL, | |||
| private readonly array $scriptAttributes = [], | |||
| private readonly ?RequestStack $requestStack = null, | |||
There was a problem hiding this comment.
Injecting the request stack here feels a bit odd, but it's the best way I can think of to handle this.
| @@ -68,6 +75,10 @@ public function render(string|array $entryPoint, array $attributes = []): string | |||
| $output .= "\n<link rel=\"stylesheet\" href=\"$url\">"; | |||
| } | |||
|
|
|||
| if (class_exists(AddLinkHeaderListener::class) && $request = $this->requestStack?->getCurrentRequest()) { | |||
There was a problem hiding this comment.
Same logic as in AbstractController
|
high-deps for asset mapper failing because 6.4 hasn't been merged up to 7.0, so FWBundle & AssetMapper tests are using not-yet-up-to-date code. |
weaverryan
force-pushed
the
asset-mapper-css-preload-weblink
branch
from
6a9d1ff
to
f17ce5b
Compare
fabpot
force-pushed
the
asset-mapper-css-preload-weblink
branch
from
9e80fbb
to
2995c16
Compare
|
Thank you @weaverryan. |
|
|
||
| private function addWebLinkPreloads(Request $request, array $cssLinks): void | ||
| { | ||
| $cssPreloadLinks = array_map(fn ($url) => new Link('preload', $url), $cssLinks); |
There was a problem hiding this comment.
I'm not sure, but shouldn't the nopush attribute be better set here? So that the css files are not sent again and again, even if they are already in client cache.
From https://developer.chrome.com/blog/early-hints/:
with HTTP2/Push it was extremely hard to avoid pushing sub-resources that the browser already had. This "over-pushing" effect resulted in a less efficient usage of the network bandwidth, which significantly hindered the performance benefits.
In current Chrome versions it wouldn't make any difference, because Server Push is already disabled there. But afaik other browsers still support Server Push in current versions.
- $cssPreloadLinks = array_map(fn ($url) => new Link('preload', $url), $cssLinks);
+ $cssPreloadLinks = array_map(fn ($url) => (new Link('preload', $url))->withAttribute('nopush', true), $cssLinks);
|
Not sure who implemented "nopush" but it seems not to be considered by most used browsers https://wpt.fyi/results/preload?label=master&label=experimental&aligned&q=nopush (compared to https://wpt.fyi/results/preload?label=master&label=experimental&aligned&q=preload` ) |
I think the browsers do not need to implement "nopush". It is the server (apache, nginx, ...) that decides based on the nopush attribute whether to push the resource or just send the preload header. |
|
See #52143 - thanks for bringing up this possible issue |
Hi!
In AssetMapper 6.4, we now know which CSS files will be rendered on the page. So, I think there is now downside to adding a
Linkheader in the response to load those CSS files, but someone tell me if I'm missing something.The only possible situation I can think of where this isn't wanted is if you decided to handle your critical CSS manually (i.e. with normal
<link rel="stylesheet">tags inbase.html.twig) and load a few less-critical CSS files through the ImportMap system. In that case, we would preload only these "less-critical" CSS, which could take a slight priority over the others. But I think the benefit this would give to most apps outweighs this. We could always add an opt-out later.Cheers!