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

Allow to pass custom headers on load responses #909

Closed
jfranciscosousa opened this issue Apr 6, 2021 · 12 comments
Closed

Allow to pass custom headers on load responses #909

jfranciscosousa opened this issue Apr 6, 2021 · 12 comments
Labels
feature request New feature or request
Milestone

Comments

@jfranciscosousa
Copy link

jfranciscosousa commented Apr 6, 2021

Is your feature request related to a problem? Please describe.
This can be related to my previous issue, which is really just some details missing in the docs #793

Despite svelte-kit having a way to define maxage to control caching for HTML pages, we really don't have a way of setting any other headers. The more common use cases are there: redirect and basic caching. But I feel like we need more control. On Next.js you can use the context object and use setHeader. Here on svelte-kit you can't even use the context because that's for layout only, so I need to resort to "hacky" solutions to get what I need.

My basic use case is to use s-maxage instead of maxage. I don't want to cache HTML pages on the client because JS and CSS hashes change between deploys, and if a user with an HTML page from a previous deploy refreshes the page (without clearing cache), the user gets an unstyled and unscripted page. Browsers make a request even if the assets are cached, but the 404 produced by the CDN (in this case, Vercel), results in those unstyled and unscripted pages. The only way around this is to probably use service workers and cache assets with it, but I want to avoid that.

I feel like a headers prop is just something that sooner or later will become handy, the same way it is on endpoints.

Describe the solution you'd like
Just allow us to specify the headers on a load response, just like on endpoints:

  export const load = async ({ fetch }) => {
    const res = await fetch("/apistuff.json");

    if (res.ok) {
      const data = await res.json();

      return {
        props: {
          data
        },
        headers: {
          "cache-control": "public, s-maxage=3600"
        }
      };
    }

    return {
      error: new Error("error during fetch")
    };
  };

Describe alternatives you've considered
I've read about hooks and managed to work around this by doing:

import type { Handle } from "@sveltejs/kit";

/**
 * Replace all max-age cache controls with s-maxage
 */
export const handle: Handle = async (request, render) => {
  const response = await render(request);

  if (!response) return response;

  const cacheControlHeader = response?.headers?.["cache-control"];

  if (cacheControlHeader) {
    response.headers["cache-control"] = cacheControlHeader.replace("max-age", "s-maxage");
  }

  return {
    ...response
  };
};

Not that pretty honestly, but works for me. This replaces all maxage usages with s-maxage which is something I'm 100% ok with, but just for this project. But this just fixes the issue for this project and this scenario, if someone wants to pass another random header, no dice.

I also tried using the context object to pass in headers during the load function and then apply them on handle, but you can't modify the context from page load functions. So that was a no-go from the start.

How important is this feature to you?
Very. Working with these recent CDN providers like Vercel, Netlify, and even Cloudflare is amazing, and being able to use s-maxage to avoid running cloud functions over and over again is just what I need, especially as I do a ton of work with local non-profits that really want to keep costs to a minimum.

@benmccann benmccann added this to the 1.0 milestone Apr 12, 2021
@matindow

This comment has been minimized.

@Conduitry
Copy link
Member

Conduitry commented Apr 25, 2021

What would happen to the headers response if this were a load that were called from client-side navigation? Would it simply be ignored? That sounds confusing.

You can also use https://kit.svelte.dev/docs#hooks-handle to add or modify response headers for any request, whether it be for a page or an endpoint.

edit: Oh, I see you mention doing this in the issue, sorry. I still don't know what I would expect to happen with these header values when load is called on the browser.

@benmccann benmccann added the feature request New feature or request label Aug 1, 2021
@benmccann
Copy link
Member

What would happen to the headers response if this were a load that were called from client-side navigation? Would it simply be ignored? That sounds confusing.

Don't we already have that though with status and maxage?

@jfranciscosousa
Copy link
Author

jfranciscosousa commented Aug 4, 2021

Yeah, technically the same thing happens when passing regular maxage, I assume it's ignored when load is called on the client-side. The behaviour of custom headers would probably be the same.

@benmccann
Copy link
Member

We talked about this in today's maintainer's meeting. Folks thought it made sense to support it on individual pages. Won't add it to layouts yet since you can use handle, other options are configured only at leaf level, and there's not a clear use case for adding it to layouts at the moment.

@nhe23
Copy link
Contributor

nhe23 commented Jan 2, 2022

I'd like to work on this issue.
My understanding on this issue is:

  • Load functions should be able to set headers (but not in layouts)
  • These headers are more or less ignored if load is run on the client-side just as status and maxage are

@nhe23 nhe23 mentioned this issue Jan 2, 2022
5 tasks
@Rich-Harris
Copy link
Member

To clarify, status and maxage are not ignored in the client. status provides the status code for the subsequent error page, and maxage causes the result of running load to be cached in memory for the appropriate length of time

@Rich-Harris
Copy link
Member

Thanks for the discussion and (@nhe23) the PR. Clearly this is an issue worth solving more elegantly (though it seems to me that the real solution is for hashed assets to be treated as immutable; this is something that adapters should probably do automatically).

Having said that, I have a nagging feeling that allowing arbitrary headers to be set in load is the wrong solution. Everything else returned from load is meaningful to both the server and the client; headers returned from load would only work for server-rendered pages, which excludes prerendered pages as well as client-side navigation. (Admittedly prerendered pages also don't respect maxage, but we could fix that since cache-control is one of the headers respected by <meta http-equiv>.) While we shouldn't approach design questions from a lowest-common-denominator perspective, I often find that these sorts of quirks have a way of becoming footguns, and are usually a sign that we haven't yet hit upon the right answer.

Besides, looking through the list of response headers on MDN, it's not clear to me that apart from cache-control there's ever a good reason for a page to have control of headers.

All of which leads me to the conclusion that we probably just need to refine our caching logic. Perhaps if we had something like this instead?

export async function load({ params }) {
  const props = await get_props(params);

  return {
-   maxage: 3600,
+   cache: {
+      public: true, // if unset, inferred using logic described in https://kit.svelte.dev/docs#loading-output-maxage
+      maxage: null,
+      smaxage: 3600
+   },
    props
  };
}

This needs some thinking through/bikeshedding (are there other things we could usefully put there?) but does this feel like a promising direction?

@nhe23
Copy link
Contributor

nhe23 commented Jan 3, 2022

It admittedly felt kind of wrong to add a new return value to the load function that is only considered server-side.
I like the idea of having a configurable cache object in the load function to solve this issue.
Maybe it would make sense to make more caching directives configurable in order to have more control over the cache-control header in general.

@jfranciscosousa
Copy link
Author

Yep, looks good to me and I agree. I'm not able to think of a scenario that would need extra headers. We can already handle redirection and caching (this new approach gives even more control, which is needed). Probably the Set-Cookie header, but you probably could do everything in the session object (still have no experience using session in svelte kit, but looks like it does the same thing).

@nhe23
Copy link
Contributor

nhe23 commented Jan 8, 2022

After some thought I think it would be a good idea if the cache-control header could be optionally set directly in the load function. As there are many possible caching directives it would make sense to set them all at once and not through a bunch of properties in the returned cache object. If no value is set for header the value for the cache-control header is inferred just the way it is today. If maxage and header are both set then maxage is used for client-side caching and header for the server-side.

export async function load({ params }) {
  const props = await get_props(params);

  return {
-   maxage: 3600,
+   cache: {
+      maxage: null,
+      header: "public, s-maxage=3600"
+   },
    props
  };
}

The cache.header value would be only cosidered server-side which is not optimal but so would be the smaxage property (and all other possible caching directive properties). Could this be a good way to solve this issue?

@benmccann benmccann added help wanted PRs welcomed. The implementation details are unlikely to cause debate and removed help wanted labels Apr 4, 2022
@Rich-Harris Rich-Harris removed the help wanted PRs welcomed. The implementation details are unlikely to cause debate label Apr 4, 2022
@Rich-Harris
Copy link
Member

Going to close this in favour of #4549

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 a pull request may close this issue.

6 participants