kit icon indicating copy to clipboard operation
kit copied to clipboard

feat: add css preload tag when prerendering

Open kevinji opened this issue 1 year ago • 9 comments

Adding a <link rel="preload"> tag to prerendering for stylesheets is helpful for static page rendering. This is especially useful when deploying to Cloudflare, which looks for preload tags with its Early Hints feature.


Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • [ ] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • [x] This message body should clearly illustrate what problems it solves.
  • [ ] Ideally, include a test that fails without this PR but passes with it.

Tests

  • [x] Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • [x] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • [x] Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

kevinji avatar May 22 '24 01:05 kevinji

🦋 Changeset detected

Latest commit: 6d9a52f57c299fb116a598df839da2bc6e385959

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar May 22 '24 01:05 changeset-bot[bot]

Looks good to me. @benmccann was there a reason we didn't use link tags to preload stylesheets even before we added Link headers in https://github.com/sveltejs/kit/pull/5735 ?

teemingc avatar May 22 '24 18:05 teemingc

I think that it's probably because that in the general case it doesn't actually do anything and just adds extra page weight since CSS is already loaded with highest priority. This is kind of a Cloudflare-specific functionality. I feel like the best solution here would probably be for Cloudflare to send early hints for all CSS rather than just preloaded CSS so that we don't have to duplicate all the CSS tags as preload tags and so that more of their customers benefit from that functionality.

benmccann avatar May 22 '24 18:05 benmccann

@benmccann That's fair, do you think it would be in-scope to have adapter-cloudflare output any Link headers in prerendered pages to Cloudflare's _headers file instead of the changes in this PR?

kevinji avatar May 22 '24 20:05 kevinji

@benmccann That's fair, do you think it would be in-scope to have adapter-cloudflare output any Link headers in prerendered pages to Cloudflare's _headers file instead of the changes in this PR?

I can see that working. We'd need to get the asset paths then add them all as a Link header key value in _headers. Maybe this can be done by reading the prerendered html files in the cloudflare adapter? or is there a better way to get them like through the Vite manifest? We also have to keep in mind the 100 rule limit for the _headers file (similar to _routes file).

  • https://developers.cloudflare.com/pages/configuration/headers/#attach-a-header
  • https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Link#specifying_multiple_links

teemingc avatar May 23 '24 13:05 teemingc

To keep the rules under the 100 rule limit, perhaps we can do some rule merging? e.g. set the header on /, and only write rules for /path if the Link header differs.

In terms of actually getting the header information out of render.js, perhaps we can write the headers out to a specific metadata file path and pick it up in the Cloudflare adapter? We may have to make sure to delete this file in other providers so there isn't any cruft in the generated output.

kevinji avatar May 24 '24 18:05 kevinji

For this PR: I think the above changes I've suggested are probably more resilient long-term but harder to work out the implementation details. In the short term, would there be support for adding in <link rel="preload"> tags instead of the headers as a configuration option, disabled by default?

kevinji avatar May 30 '24 16:05 kevinji

@dario-piotrowicz any chance you might be able to put in a request to have Cloudflare send early hints for all CSS rather than just preloaded CSS? (see above for context: https://github.com/sveltejs/kit/pull/12247#issuecomment-2125528274. happy to clarify on Discord if that's easier)

benmccann avatar Jun 19 '24 20:06 benmccann

Dario suggested an interesting answer that might be the way to go here. I tested and, at least in firefox and chrome where I tested, you don't need both a tag to preload it and a tag to load it. You can simply preload it and that is enough.

That would break IE users, but that ship may have sailed. Perhaps we should user agent sniff and not do it there though? Might have to take a look at https://github.com/sveltejs/kit/pull/6265 to see how badly broken we are in IE right now

benmccann avatar Jun 19 '24 23:06 benmccann

@benmccann Sorry for the super delayed response. I updated this feature to be gated behind a config option that defaults to false, since I'm not super confident about changing the default behavior to only emit using preload tags with the standard stylesheet <link> tags. Is this reasonable for merging?

kevinji avatar Sep 14 '24 22:09 kevinji

LGTM. @benmccann what do you think? It has a config option defaulting to false at the moment so it shouldn't break anything.

cc: @jamesopstad https://github.com/sveltejs/kit/pull/12247#issuecomment-2179449663

teemingc avatar Oct 06 '24 02:10 teemingc

We've generally tried to avoid adding options to reduce user complexity. I think always adding the preload in place of the normal css loading would be preferable. The framework should take care of these decisions for users. By creating an option we just add decisions that users are responsible for making

benmccann avatar Oct 06 '24 04:10 benmccann

Okay, will close based on discussion above.

kevinji avatar Nov 07 '24 04:11 kevinji