Replace `this.fetch` with `this.get`

[Rich-Harris]

Stumbled into a bit of a rabbit hole while adding prerendering to adapter-node — because this.fetch is expected to work exactly the same as fetch, we have to account for the possibility that someone will this.fetch('/some-static-file.json), which means that the prerenderer needs to know where the static files could live, which muddies the API somewhat.

It's also just a bit of an awkward API:

• Sapper doesn't currently handle URLs relative to the requested page (though this could easily change, and has in fact changed in the current app-utils)
• The whole res = await this.fetch(...); data = await res.json() dance is a bit of a pain, honestly
• It's easy to forget that you need credentials: 'include' if you're using session
• fetch supports non-GET methods, which don't make sense in the context of preload (or load, as the case may be)

I'd like to replace it with this.get:

<script context="module">
  export async function load({ params }) {
    return await this.get(`./${params.slug}.json`);
  }
</script>

• Only works with server routes, not static files or external resources. Simpler
• Accepts a URL (easiest way to identify server routes, which will of course continue to work with plain fetch), which can be relative to the requested page
• Always includes credentials
• Returns a promise for the body of the response. If the Content-Type is application/json, is JSON.parsed. If it's text/*, is returned as text. Otherwise is returned as an array buffer
• Non-200 responses throw an error (augmented with status code), which is equivalent to calling this.error(status, error), except that you can catch and handle if necessary. For common cases, this eliminates the need to add error handling around the response returned from this.fetch

We could also expose get alongside the other app functions (goto et al) for use outside preload/load functions. (It would probably just throw in Node.) That's not necessary for this.get to have value though.

Of course there are still times when it's useful to be able to use fetch in preload/load — hn.svelte.dev fetches data from https://api.hnpwa.com, for example, and it's good to be able to do this isomorphically (rather than forcing the app to always connect to the app's own server, like ahem some frameworks). But if it's only to be used for external URLs, then there's no need to use this.fetch, we can just use regular fetch. We can polyfill it in Node with @rollup/plugin-inject and node-fetch (which already powers this.fetch). With this approach, many apps wouldn't need node-fetch at all, whereas currently it has to be included because of this.fetch.

This seems like all upside to me, except for the breakingness of the change. But if there was ever a time for breaking changes this is it. Thoughts?

[benmccann]

It's easy to forget that you need credentials: 'include' if you're using session

Do you still? I thought that was a bug we fixed in Sapper 0.27.14 - sveltejs/sapper#882. I'm not sure how session is related though, so maybe I'm missing something

[Rich-Harris]

Ah, I didn't realise we'd made that change

[Conduitry]

I think another one of the reasons for this.fetch was because on the server it needed extra request-specific handling for session stuff. How would that work? Even if the plugin injects it on a per-module basis, we'd still need to tie it back to the original request somehow.

[Rich-Harris]

All that complexity exists so that you can make credentialled server-side fetches to your own server routes. I don't think it makes sense to do fetch('https://other.site/stuff', { credentials: 'include' })?

So all this.get needs to do in order to maintain the session is to forward cookies from the request for the page itself

[benmccann]

There's a PR open to do that with fetch in Sapper: sveltejs/sapper#1478

[arxpoetica]

I'm inclined to agree that this is all upsides, i.e., a built in API. As long as there's an escape hatch (which it appears there is) for loading up your own fetch or whatever.

[arxpoetica]

I do wonder about tying it to this. That possibly seems like a mistake. Might be better as an import.

<script context="module">
  import { fvetch } from 'svelte/kit';
  export async function load({ params }) {
    return await fvetch(`./${params.slug}.json`);
  }
</script>

Regardless, it must be called fvetch.

[Rich-Harris]

It can't work as an import, it needs context

[Rich-Harris]

@benmccann forwarding cookies from the request already happens, I think that PR is about cookies going in the other direction?

[Rich-Harris]

@lukeed pointed out that fetch defaulting to credentials: 'same-origin' defeats CDN-level caching. Luke argues that this is a point in favour of this.fetch, since you can already do credentials: 'omit' to make things more cacheable, but the fact that most of us aren't actually doing that is evidence that this may not be the approach that leads to the best user outcomes — most people simply aren't thinking about this stuff.

I think it's a point in favour of this.get (or this.request, if we want to support other methods for the sake of GraphQL...), since it could be smart: if the requested server route uses session...

export async function get({ params }, session) {
  return session.user
    ? await db.get(build_query(params, session.user))
    : { status: 403 };
}

...then we make a credentialed fetch, but if not...

export async function get({ params }) {
  return await cms.get(params.slug);
}

...then we don't.

This feels like a good 99% solution. For the remaining 1% (though I'm struggling to think of what that case would look like) we could have a second argument containing the relevant options:

this.request(`./me.json`, {
  credentials: 'same-origin'
});

[lukeed]

To an earlier comment, you don't need context. Unless I'm missing context, we have all the context we need at build-time. At build, you already know if youre server or client bundle, and so in server builds, the import can be one thing and in client it can resolve to another.

I 100% think it should be import-based. Any this.get or this.fetch is going to be constrained and/or you/we'll have to do so much juggling that it might as well be another library

[Rich-Harris]

I don't just mean server or client context, I mean context of which page is currently being requested. And in the SSR case, you need the context of which user is making the request, otherwise you can't attach credentials.

[lukeed]

Those could be passed in as parameters to preload. IMO that's not enough of a reason to force someone's choice.

[Rich-Harris]

Where I'm coming from with this is that I think we should make the 99% case extremely simple and the 1% case possible. It feels like what you propose makes the 1% easier, but the 99% harder (though I expect you would dispute the 99 and the 1).

For example this...

export function load({ params }) {
  return this.get(`./${params.slug}.json`);
}

...is much nicer in my view than this:

import { request, resolve } from '@sveltejs/kit';

export function load({ path, params }) {
  const href = resolve(path, `./${params.slug}.json`);

  return request(href, {
    credentials: 'omit'
  });
}

What are the counter-examples?

[lukeed]

Using relative URLs is general is truly not that common. Literally the only place I continually see it is in the Sapper docs and Svelte templates. And we saw early on how error prone this is (because it is) when users started adding deeper paths into their SPA apps – eg, <script src="bundle.js"></script> when loading from /blogs/hello-world would 404 because it tried to load blogs/bundle.js. Absolute URLs are far more common because theyre safer – in both client and server – since they resolve absolutely and can survive basic refactors.

That said, a modified 2nd snippet is 10x better, for users of any type (I've routinely seen clients/coworkers say "wtf where does this.fetch come from? i don't trust it")

import { fetch } from '@sveltejs/kit';
// or send from httpie

export function load({ params }) {
  // omitted by default
  return fetch(`/blog/${params.slug}.json`);
}

export function load({ path, params }) {
  // i guess i need a relative request, not sure why
  const { href } = new URL(`./${params.slug}.json?private=1`, path);
  return fetch(href, { credentials: 'include' });
}