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

Add serverFetch hook #1465

Merged
merged 10 commits into from
Jun 28, 2021
Merged

Add serverFetch hook #1465

merged 10 commits into from
Jun 28, 2021

Conversation

stalkerg
Copy link
Contributor

This PR base on #1417 (comment) and on all discussion for direct external API requests (#1417) and headers manipulation from #696.

Basically, now we provide developers full control of how to retrieve data on the server-side and even UNIX sockets can be an option.
The test implementation is a little tricky but looks like we have already a similar thing for handles large responses.

Thanks.

Copy link
Contributor

@Kapsonfire-DE Kapsonfire-DE left a comment

Choose a reason for hiding this comment

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

the type ServerFetch have to be capitalized to fit the other namings

export { Incoming, GetSession, Handle, ServerResponse as Response, serverFetch } from './hooks';

@pixelmund
Copy link
Contributor

This looks cool but right now, let's say i have an external api which depends on users cookies, the cookies only gets send through internal sk endpoints while ssr even if its the same domain. Couldn't we pass the 'server' request to the serverFetch hook? I would currently have to patch package svelte kit to pass request headers to the external api or create an sk endpoint which proxies the request.

@stalkerg
Copy link
Contributor Author

Couldn't we pass the 'server' request to the serverFetch hook?

I also thought about it... it will be greater to have, but I am not sure about @Rich-Harris opinion. In that case, we will break the fetch API.

@stalkerg
Copy link
Contributor Author

@Kapsonfire-DE thanks! Fixed.

@benmccann
Copy link
Member

I haven't gotten a chance to take a look at this one to think about whether I think this is the best solution or not yet, but it would probably need the documentation to be updated as well

@stalkerg
Copy link
Contributor Author

stalkerg commented May 17, 2021

@benmccann ok, I will prepare it. Also, my project has already used it with good results. Also, please check @pixelmund idea, probably serverFetch shouldn't reproduce fetch API.

@stalkerg
Copy link
Contributor Author

I added some documentation but I think changelog will be better to add after final approves of the concept.

@benmccann benmccann added the feature request New feature or request label May 19, 2021
```js
/** @type {import('@sveltejs/kit').ServerFetch} */
export async function serverFetch(request) {
const data = await getDataMethod(request);
Copy link
Member

Choose a reason for hiding this comment

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

you could do this in one line:

return await getDataMethod(request);

or did you mean for the next line to be return new Response(data) as in Rich's example? #1417 (comment)

it's unclear what getDataMethod is here though. maybe we should show a more realistic example such as changing the URL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we should show a more realistic example such as changing the URL

ok, I also thought about it.

@@ -187,7 +187,8 @@ class Watcher extends EventEmitter {
},
hooks: {
getSession: hooks.getSession || (() => ({})),
handle: hooks.handle || (({ request, render }) => render(request))
handle: hooks.handle || (({ request, render }) => render(request)),
serverFetch: hooks.serverFetch
Copy link
Member

Choose a reason for hiding this comment

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

should this also provide a default implementation like serverFetch: hooks.serverFetch || fetch? then you can just call it directly without having to check if it's defined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm maybe it's a good idea, I can check it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! During these changes, I found and fix the serious issue.

@SGarno
Copy link

SGarno commented May 21, 2021

Kudos for getting this going. I was struggling the logic flow with how SK handles requests and getting that to work with an app that only connects with external APIs (no frontend page protection).

Question with this new serverFetch method....what is the pattern for capturing cookies?

I have an external API that operates in this way:

  1. Call an endpoint to identify the type of authentication that is configured on the backend server. This returns a session_id cookie.
  2. Depending on the response value, it may require username/password, some kind of existing authentication token (i.e. SAML), auto-negotiate, or OAuth2.
  3. If OAuth2, we have to display their authentication page to capture the token and update the cookies accordingly.

I am building a wrapper class library for this API that will take care of all the external api communication (not using routes).

Will headers be available to the serverFetch without having to put code in the frontend (i.e hooks.js and routes)?

@stalkerg
Copy link
Contributor Author

Will headers be available to the serverFetch without having to put code in the frontend (i.e hooks.js and routes)?

The serverFetch reuses the Request object from your fetch call in load function and you can copy your headers into a new request. Probably you are talking about @pixelmund idea?

@SGarno
Copy link

SGarno commented May 22, 2021

I realize @pixelmund was also asking about headers, but wasn't sure if this enhancement addressed that or not from the comments I read above (which is why I was asking).

In my use case, the application connects to multiple external APIs (i.e. Salesforce, Snowflake, etc.) not for access to the application itself, but for usage of the systems it interacts with. For this to go through the current hooks and /route endpoints makes writing a wrapper API class awkward in that it has to depend on a tight frontend integration (which is what I am trying to avoid by creating a wrapper class in the first place).

If this serverFetch is not going to address this use case, should I open an enhancement request?

My original thought was to propose something like the following:

export default class MyAPI {

    #baseurl;
    #data;

    async Init (url) {
        this.#baseurl = url + "/api/v1/";

        let url = this.#baseurl + 'Server'
        let resp = await fetch(url, {
            method: 'GET',
            render: 'client',    // (could also be 'server' but headers will be undefined)
            headers: {
                Accept: 'application/json',
                'Content-Type': 'application/json',
            },
            credentials: 'same-origin',
        })

        if (resp.status != 200) {
            console.log(`${url} => ${resp.status}:${resp.statusText}`)
            throw new Error(`Unable to connect to server at specified URL [${this.#baseurl}]`)
        }

        // Available in this wrapper class when render = 'client'
        let sessionID = resp.headers.cookie('session_id')

        this.#data = await resp.json()
    }
}

When the render option of 'client' is received by SK, it will force the fetch request to be executed client side so that the cookies can be returned back to the caller without requiring boilerplate-y code in hooks and routes. If the value is 'server' (default) then it would operate the same as it does today.

Thoughts?

@stalkerg
Copy link
Contributor Author

stalkerg commented Jun 2, 2021

ok, I will back soon

@stalkerg
Copy link
Contributor Author

stalkerg commented Jun 4, 2021

If this serverFetch is not going to address this use case, should I open an enhancement request?

serverFetch can help here in your case, because you can write your communication with external API directly without proxy on the SvelteKit side.

@SGarno
Copy link

SGarno commented Jun 4, 2021

serverFetch can help here in your case, because you can write your communication with external API directly without proxy on the SvelteKit side.

Excellent! 👏 I am curious to see how it operates with an OAuth2 credentialed API. If you have any examples of how we would render (the "SK way") the authentication url to the client using serverFetch, that would be really helpful.

@SGarno

This comment has been minimized.

@benmccann

This comment has been minimized.

@Rich-Harris Rich-Harris merged commit d874dbd into sveltejs:master Jun 28, 2021
@Rich-Harris
Copy link
Member

finally got round to reviewing/merging this. thank you!

@istarkov
Copy link
Contributor

Related #1784
This breaks the node-adapter sveltekit builds

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 this pull request may close these issues.

9 participants