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: supports link header #4785

Closed
wants to merge 2 commits into from
Closed

feat: supports link header #4785

wants to merge 2 commits into from

Conversation

maraisr
Copy link

@maraisr maraisr commented Apr 30, 2022

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 pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

This is my attempt at solving #4783 albeit not specifically for Cloudflare, but think all servers could benefit from it.

Probably this PR won't be solution that'll land as only SSR'd non-preloaded pages will have this header. But really all static html files could use it.

So, my thinking lies more about exposing the assets consumed by a page to an adaptor.

Adaptors know how to deal with their targets, as some use a _headers file, others allow it as well as new Response('', {headers}), and other are entirely different. So maybe instead of this functionality being first-class, defer it to an adaptor.

This way Cloudflare's adaptor can build a _headers file to contain the headers for all static (pre-rendered) html files, as well as include the Link header for ssr responses.

Will mark PR as draft, but feel free to open and merge if you see fit.

@changeset-bot
Copy link

changeset-bot bot commented Apr 30, 2022

⚠️ No Changeset found

Latest commit: cfb19fb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@benmccann
Copy link
Member

I wonder if we should not include the modulepreload in the HTML if the header is present? I'm curious what browsers this feature is supported in. MDN doesn't have any data for this and I thought I remembered the feature or a similar one being removed from browsers

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — is there any reason we wouldn't just always include this header? Would be simpler if we didn't have to introduce another config option here

documentation/docs/14-configuration.md Outdated Show resolved Hide resolved
documentation/docs/14-configuration.md Outdated Show resolved Hide resolved
documentation/docs/14-configuration.md Outdated Show resolved Hide resolved
packages/kit/src/runtime/server/page/render.js Outdated Show resolved Hide resolved
packages/kit/src/core/dev/plugin.js Outdated Show resolved Hide resolved
packages/kit/types/internal.d.ts Outdated Show resolved Hide resolved
@maraisr
Copy link
Author

maraisr commented May 4, 2022

Thanks for that one @Rich-Harris I have updated the PR — and you're right, I cannot see why this header SHOULD not have been included by default.

Have added that in.

As per other points mentioned around what do we do for pre-rended files, do we need something similar to this by in lastmile? Aka post process once that file is either read from disk, from KV or so forth?

@benmccann
Copy link
Member

This PR doesn't actually implement early hints as discussed in the linked issue (e.g. nowhere are we returning a 103 status code). What this PR does do is implement headers for server push is which is being removed from browsers. I don't think this PR would necessarily hurt, but I think the benefits are likely to be minimal. The benefits might be meaningful for a page with high SSR render time if early hints were indeed implemented

From https://medium.com/reloading/preload-prefetch-and-priorities-in-chrome-776165961bbf:

Preloading via headers may offer a very negligible improvement, but require the document to be committed before processing happens.

You can provide preload links in either form, but there is one important difference you should understand: as allowed by the spec, many servers initiate an HTTP/2 Server Push when they encounter a preload link in HTTP header form. The performance implications of H/2 Server Push are different from those of preloading (see below), so you should make sure you don’t unintentionally trigger pushes.

From https://www.ctrl.blog/entry/http2-push-chromium-deprecation.html:

Chromium developers have announced that they plan to remove support for HTTP/2 server push from the market-leading browser engine.
The Chrome team has measured the adoption of HTTP/2 push, and found that only 0,05 % of all HTTP/2 sessions seen by Chrome users use server push.
Servers aren’t aware of the state of the client’s cache. They can’t know what resources the client has stored from previous visits, and risk pushing assets needlessly. This can waste people’s precious data quotas, slow-down page-loading performance, and cost you more money.

From https://brianli.com/2020/12/chrome-to-drop-support-for-http2-server-push/:

Firefox and Safari both support HTTP/2 push and we are not aware of any plans for removal.

@maraisr maraisr closed this May 4, 2022
@maraisr
Copy link
Author

maraisr commented May 4, 2022

Sorry to have caused you trouble.

@maraisr maraisr deleted the support-link-header branch May 4, 2022 22:10
@mrkishi
Copy link
Member

mrkishi commented May 5, 2022

@maraisr Thank you for investigating this. I believe this PR could still move forward (although possibly with changes).

I think there's some miscommunication at play here.

Hints can be added both to Link: headers and <link> tags, and they should indeed provide similar performance. However, headers are more convenient when doing further optimization since they arrive early and don't require an html parser... And that's why they were once used for HTTP/2's Server Push (which while deprecated is still supported by some servers and clients) and they're similarly being used for Early Hints by reverse proxies.

Now for the kicker: Link: headers on responses with status codes 200, 301 and 302 are cached and reused by Cloudflare to implement Early Hints. Even if we don't explicitly implement 103 status codes, make Svelte 4's render() streaming and turn SvelteKit into a streaming-first framework, users can still benefit from link headers when paired with the right reverse proxy with some configuration.

Since we already have <link> tags, adding the equivalent header shouldn't cause issues—although because of Server Push it could change the performance characteristics of some proxy/client combos, so perhaps it'd be safer to add a nopush hint.

Apart from the nopush hint, it's possible we'd need as=style for css, although I'm not sure it makes a difference these days.

But would adding this to SSR now become a chore later if we decide to move the responsibility to adapters (as noted in OP)?

@Rich-Harris
Copy link
Member

I think we should reopen this PR until we've discussed this further, but I'm unable to do so - @maraisr can you hit the reopen button please? Thanks 🙏

@Rich-Harris
Copy link
Member

Some further relevant discussion about flushing JS/CSS-related tags (basically everything in src/app.html up until %svelte.head%, plus those tags) as soon as we match a route, rather than after the route has been rendered: #4831 (reply in thread).

Also related: #4158 (comment)

@mrkishi
Copy link
Member

mrkishi commented May 5, 2022

Since early headers would require solving the bigger streaming questions (early status codes) it sounds like it could continue to be discussed in #4831, and for now we just focus on improvements assuming late headers. In that case, this seems mostly there as a stop-gap, except for the adapter-specific questions. Or should we focus on the full streaming solution?

Regarding the adapters, do you have opinions on where to take the adapter api in order to expose this sort of per-page information? For instance, it'd be nice if they were able to setup their platform's necessary plumbing to add the headers to prerendered pages.

@benmccann
Copy link
Member

Now for the kicker: Link: headers on responses with status codes 200, 301 and 302 are cached and reused by Cloudflare to implement Early Hints.

Ah, I wasn't aware of that. Thanks for sharing. Although, it sounds like the impact is quite limited when the page has been cached and the early hint may not be sent at all:

"Early Hints may be emitted less frequently on requests where the content is cacheable. Cloudflare CDN is more likely to retrieve a response header before the asynchronous Early Hints lookup finishes if the response has been cached. Cloudflare will not send a 103 response if the main response header is already available."

perhaps it'd be safer to add a nopush hint

I think I'd lean towards this so that all the browsers work the same. Most testing and development is probably done in Chrome, so it probably makes sense to have the other browsers work the same way.

But would adding this to SSR now become a chore later if we decide to move the responsibility to adapters (as noted in OP)?

I don't think this would really make sense to live in adapters since it's a runtime rather than build-time concern?

@mrkishi
Copy link
Member

mrkishi commented May 5, 2022

it sounds like the impact is quite limited when the page has been cached

Yeah, that's expected. When a page is cached it makes sense not to use early hints since they wouldn't be early as they wouldn't be waiting for origin. But since this is regarding SSRed pages, most won't be cached. And iirc Cloudflare also doesn't cache html pages by default even with cache-control headers, you have to add a Page Rule for that.

I don't think this would really make sense to live in adapters since it's a runtime rather than build-time concern?

I wasn't clear there, but this is a concern when prerendering pages. Granted, the upside is limited for the same reason as Cloudflare won't use early hints when cached, but this is something that'd impact other headers anyway, so it's worth considering how it'd be handled anyway. We currently have no way to specify headers on prerendered pages outside of meta tags.

e: Come to think of it, this would probably be doable for all headers as-is, right? We wouldn't need a separate api for SSR to specify additional data; whatever headers are in the response could be passed on to the adapter to deal with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants