kit
kit copied to clipboard
feat: add css preload tag when prerendering
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 testand lint the project withpnpm lintandpnpm 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 changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.
Edits
- [x] Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.
🦋 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
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 ?
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 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?
@benmccann That's fair, do you think it would be in-scope to have
adapter-cloudflareoutput anyLinkheaders in prerendered pages to Cloudflare's_headersfile 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
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.
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?
@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)
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 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?
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
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
Okay, will close based on discussion above.