Export and cross-fetch don't play nice together

[arxpoetica]

[Is your feature request related to a problem? Please describe.]
I spent a day and a half trying to figure out why export wasn't crawling any of my *.json.js files. The files were generated on export, just not crawled. These are from fetch API calls in preload functions/scripts.

I'm tempted to call this a bug, but I discovered that when I started using this.fetch instead of cross-fetch, it started working correctly. So technically this isn't a bug if you use the preloader this.fetch API correctly.

[Describe the solution you'd like]
But that makes creating fetch API wrappers hard. I either have to explicitly pass this.fetch as an argument to a wrapper function. (I create reusable GET and POST utility libraries/functions). Or I have to write out the same boilerplate again and again.

I don't have a strict recommendation—maybe one would be to make this.fetch universally available—that's problematic too though. I'm tempted to say, let's actually strip out the this.fetch feature entirely, recommend using something like cross-fetch in its place, and then just fix that "bug."

[Describe alternatives you've considered]
None.

[How important is this feature to you?]
Dunno. Maybe this is a "just me" issue, and it should be closed—maybe no one else will run into this. I thought it was worth discussing though.

[swyxio]

i think this neglects the reason this.fetch exists in the first place. it argues for better documentation on why sapper needs you to use this.fetch.

[Conduitry]

Something else that's come up as a possibility is allowing users to attach their own stuff to the this object that preloads are called with, so you can have your own helpers that still use this.fetch internally. The question then would be where this ought to live, since we'd need to attach this stuff both on the server and the client.

[Conduitry]

I'm tempted to say, let's actually strip out the this.fetch feature entirely, recommend using something like cross-fetch in its place, and then just fix that "bug."

This isn't particularly viable as far as I can tell, as the this.fetch behavior needs to be rather different on the server and on the client in order for it to be called identically in both places and just work. On the client, it's just a regular fetch, but on the server, you can see all the extra stuff we have to do in the get_page_handler.ts file.

[arxpoetica]

as the this.fetch behavior needs to be rather different on the server and on the client

Agreed. Arguably, this.fetch is doing it's job perfectly reasonably, without too much boilerplate, which is actually a good API.

I think my complaint is probably both a valid one, but arguably we also probably shouldn't do anything about it (necessarily). I mostly just wanted to explore the options. I'll probably close this in a day or two, unless someone comes up with a really bright idea on how to make it easier to wrap this.fetch.

[arxpoetica]

i think this neglects the reason this.fetch exists in the first place. it argues for better documentation on why sapper needs you to use this.fetch.

On second thought, this makes a good point. I even knew this.fetch was behaving differently on client and server—but I've noticed it's a Q that comes up all the time in the forums. "What can I use for server side fetching." And this.fetch isn't available anywhere outside of preload.

I wonder if it shouldn't be attached to this but instead should be something along these lines:

import { fetch } from `@sapper/app`

To be available everywhere?

Then when people ask which fetch lib to use, we just tell them to use the internal one...???

[swyxio]

again, that would likely be impractical if you thought through the implementation. imports are by design far more static than this context assignment.

keep ideating tho, i can see you’re exploring different solutions bc it doesn’t feel right to you.

perhaps more compiler magic...?

[Conduitry]

Yeah, using an import from a module for this is also not too viable. You'd need the import to be different things on the client and on the server (which is doable, but fiddly), but on the server side you'd also need the import to somehow be aware of the relevant req and res objects from the server instance (which is considerably less doable, especially since all of this stuff can be async).

It's a lot simpler to just call preload with a certain context containing an appropriately configured and wrapped node-fetch. Having this only be available in preload isn't too much of a restriction, because that's the only place you really should be doing fetches from on the server. Anything else, which happens only on the client, can happen inside an onMount or whatever, and will also have access to the browser's ordinary fetch.

[thgh]

+1 for preload helpers.

Or maybe this pattern:

import helpers from 'src/lib/preload-helpers'
export const preload = helpers(({ params }) => ...)

Could preload be a hook that receives context?

[nowylie]

Would it be possible to use VM or vm2 to execute the preload function on the server side injecting this.fetch into a global context?

I think this would allow you to use the same wrapper code on both client/server side without worrying about where the global fetch comes from.

[Conduitry]

The server's this.fetch has things specific to the current request and session, so exposing that beyond its intended scope is not a good idea.

[nowylie]

I understand that, sorry if I wasn't clear.

I'm proposing that in the handle_page function on the server side you could use vm.createContext to create a global context with the per-request fetch function inside it and then call the route preload functions using this context.