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

Direct communication with external API #1417

Closed
stalkerg opened this issue May 11, 2021 · 18 comments
Closed

Direct communication with external API #1417

stalkerg opened this issue May 11, 2021 · 18 comments

Comments

@stalkerg
Copy link
Contributor

The main idea is getting a shorter path to API from SSR and CSR without extra hops and proxies.
For that, we should have different server URLs and client URLs for the same resource ID. Even now you can have different URLs for fetch between server and client but our serialization system detect resource by the whole URL and produce double request in that situation.
This feature request is not something new and several times accrued in Sapper tracker for example sveltejs/sapper#489
Also, I did some rouge PR for this here #1406, and you can find some extra details there.
It looks like we should more detail describe this feature and cases to make a clean solution.

It's a little confusing theme, but I want to hear your opinions and ideas.

@Kapsonfire-DE
Copy link
Contributor

Kapsonfire-DE commented May 11, 2021

I didnt checked in detail - but i liked the initial idea of defining a hash in the fetch method instead of using the url as identifier
The argument not to change the default fetch parameters are strong and I agree to this.

So we have different ways to accomplish this i can think of

  1. give the fetch a prototype function which returns a wrapped fetch method with defining the hash method
fetch.customHash('myCustomHash')('/my/endpoint/url', {...})

probably one of the ugliest ways

  1. Like 1, but with import
import {hashedFetch} from "svelte";
hashedFetch('/my/endpoint/url', {...}, 'myCustomHash')
  1. Create nasty handle/endpoint hacks which creat additional network request on CSR.
    While this works out of the box - other approaches are much cleaner

  2. Define in config a host, which get automatically prepended on fetch requests
    At current state the initialFetch uses current svelte kit host variable. I dont know if its used in other functions too.
    But using a config like this could work:

{
   kit: {
        fetch: {
            prependHost: 'http://10.0.10.3:1337/'
         }
   }
}

While this could be a good solution, whats about when different Hosts should be prepended.

  1. Add an additional hook in hooks.ts/hooks.js which allows to intercept the server fetch method
   export function interceptServerFetch(url, params, fetch) {
   if(url.startsWith('/')) url = 'http://10.0.10.3:1337' + url;
   return fetch(url, params);
}

While fetch is often used in load, option 1 and 2 will leak this information to the browser.
4 feels good, but is very static

version 5 feels superior IMO

@stalkerg
Copy link
Contributor Author

stalkerg commented May 11, 2021

I like 5 also but it has two issues:

  1. Not so flexible as 1 and 2
  2. If you have an extra prefix for client URL like /api/ '/api/my/endpoint/url' you should replace it instead of just concatinate. It looks dirty.

In the same time 5 will be very easy to implement.

@Kapsonfire-DE
Copy link
Contributor

You could do this in any way you like, but keep in mind, the url has to be valid for CSR. You're gonna define if its replaced, concatinated or something like this

export function interceptServerFetch(url, params, fetch) {
   if(url.startsWith('/api/')) url = 'http://10.0.10.3:1337/' + url.substr(5);
   return fetch(url, params);
}

about not so flexible - I guess I have to disagree. The only thing thats not possible is to give two different urls the same hydration.

@Kapsonfire-DE
Copy link
Contributor

Kapsonfire-DE commented May 11, 2021

But to fit more your example you wanna do something like this
MyComp.svelte

export async load({fetch}) {
    let res = await fetch('https://api.domain.com/endpoint');

.... 
};

hooks.js

export function interceptServerFetch(url, params, fetch) {
   url = url.replace('https://api.domain.com', 'http://10.0.10.3:1337');
   return fetch(url, params);
}```

@stalkerg
Copy link
Contributor Author

stalkerg commented May 11, 2021

Ok, it's looking good enough and easy to implement.
PS also it's can be helpful for geo deploying when your browser should get access to closest API by GeoDNS but server part can touch neighborhood server or same server.

@benmccann
Copy link
Member

That's basically what Rich had suggested in #1406 (comment) with his mutateFetch suggestion

@stalkerg
Copy link
Contributor Author

@Rich-Harris then can I try to implement it?

@Kapsonfire-DE
Copy link
Contributor

That's basically what Rich had suggested in #1406 (comment) with his mutateFetch suggestion

Yeah I did it more aggressive returning a promise like the normal fetch method. this way, its also possible to resolve it on different ways like unix sockets, file reads or other socket connections

@Rich-Harris
Copy link
Member

Oh, the custom fetch replacement idea is really interesting. Lots of flexibility there. I would maybe suggest this interface:

/**
 * @param {RequestInfo} info
 * @param {RequestInit} init
 * @returns {Response}
 */
export async function serverFetch(info, init) {
  // potential footgun: RequestInfo can be a string or a Request object with a url property
  // — people would need to decide for themselves whether to accommodate the latter
  const data = await get_data_somehow(info);
  return new Response(data); // https://developer.mozilla.org/en-US/docs/Web/API/Response/Response
}

There's no need to pass fetch in as it's globally available inside a SvelteKit app (along with Response, Request and Headers).

@Kapsonfire-DE
Copy link
Contributor

Isnt the fetch method (given by load (context module)) needed to simulate the "user" request?

@Rich-Harris
Copy link
Member

I don't understand?

@Kapsonfire-DE
Copy link
Contributor

To transport the headers of the initial request of the browser to the endpoints, like cookies/sessionIDs and so on?

@Conduitry
Copy link
Member

The fetch function passed to load functions is a separate wrapper around the global fetch function that's being discussed here. The serverFetch hook would already be getting called with the appropriate cookie HTTP headers, etc.

@Rich-Harris
Copy link
Member

There's an issue around browser-sent request headers here: #696. I think what we'd need to do here is take the initial info, init and normalise them into a Request object that includes whichever request headers we decide to include.

So perhaps the interface is actually more like this, which conveniently deals with the footgun mentioned above:

/**
 * @param {Request} req
 * @returns {Response}
 */
export async function serverFetch(req) {
  const data = await get_data_somehow(req.url);
  return new Response(data); // https://developer.mozilla.org/en-US/docs/Web/API/Response/Response
}

@stalkerg
Copy link
Contributor Author

I think creating a Request object each time is a good idea instead of trying to use string URL from time to time.
Main point here - serialization and resource ID things will be outside of real network code and it's good.

@benmccann
Copy link
Member

In reviewing #1465, I think there's a difficulty with the proposed API, which is that SvelteKit does not call fetch for any internal calls but instead resolves the requested resource in-process to replicate fetch as a performance optimization. As a result, #1465 is only working on external URLs, which I think is not a great experience or API.

One difficulty with extending it to work on the internal API is that we'd lose the performance optimization. E.g. consider the code below. It would call the node-fetch implementation instead of our custom implementation that is passed into load and avoids making an HTTP call.

export async function serverFetch(info, init) {
  info = modify_url(info);
  return fetch(info, init);
}

Something akin to @Rich-Harris's earlier mutateFetch suggestion in #1406 (comment) seems easier to implement though I do still like the idea of serverFetch

Perhaps we can modify the serverFetch hook API to also provide SvelteKit's fetch implementation:

export async function serverFetch(info, init, fetch) {
  info = modify_url(info);
  return fetch(info, init);
}

@stalkerg
Copy link
Contributor Author

@benmccann I suppose this optimization for "requested resource in-process" not so important here, much more important it provides serialization to avoid double load on the browser side.

Perhaps we can modify the serverFetch hook API to also provide SvelteKit's fetch implementation:

It's can work if we will do serialization outside this function. For that, we should split our server fetch function. If you want I can provide this type of PR. But can I hear @Rich-Harris opinion?

@benmccann
Copy link
Member

Rich pointed out to me that my use case of redirecting fetches on the same domain to another location (e.g. app server written in another language) can already be done with handle. And because requests to the same server don't result in an extra hop there's no extra latency, etc.

export async function handle({ request, resolve }) {
  if (request.path.startsWith('/api') {
    return fetch_from_api(request);
  }

  return resolve(request);
}

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

No branches or pull requests

5 participants