Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add option for css preloads when prerendering #12247

Closed
wants to merge 1 commit into from

Conversation

kevinji
Copy link
Contributor

@kevinji kevinji commented May 22, 2024

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
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • 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

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

Copy link

changeset-bot bot commented May 22, 2024

🦋 Changeset detected

Latest commit: 6d9a52f

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

@eltigerchino
Copy link
Member

eltigerchino commented May 22, 2024

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 #5735 ?

@benmccann
Copy link
Member

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.

@kevinji
Copy link
Contributor Author

kevinji commented May 22, 2024

@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?

@eltigerchino
Copy link
Member

eltigerchino commented May 23, 2024

@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).

@kevinji
Copy link
Contributor Author

kevinji commented May 24, 2024

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
Copy link
Contributor Author

kevinji commented May 30, 2024

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?

@benmccann
Copy link
Member

@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: #12247 (comment). happy to clarify on Discord if that's easier)

@benmccann
Copy link
Member

benmccann commented Jun 19, 2024

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 #6265 to see how badly broken we are in IE right now

@kevinji kevinji force-pushed the patch-1 branch 2 times, most recently from c21e56f to 16ba0ff Compare September 14, 2024 22:50
@kevinji kevinji changed the title feat: add css preload tag when prerendering feat: add option for css preloads when prerendering Sep 14, 2024
@kevinji kevinji force-pushed the patch-1 branch 2 times, most recently from e47fa69 to d1080ff Compare September 14, 2024 22:55
@kevinji
Copy link
Contributor Author

kevinji commented Sep 14, 2024

@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?

`KitConfig.prerender.generateCssPreloadTags` will configure whether
preload tags for stylesheets are generated during prerendering.
@eltigerchino
Copy link
Member

eltigerchino commented Oct 6, 2024

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 #12247 (comment)

@benmccann
Copy link
Member

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

@kevinji
Copy link
Contributor Author

kevinji commented Nov 7, 2024

Okay, will close based on discussion above.

@kevinji kevinji closed this Nov 7, 2024
@kevinji kevinji deleted the patch-1 branch November 7, 2024 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants