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

Rename handle's render parameter to resolve #1566

Merged
merged 3 commits into from
May 29, 2021
Merged

Rename handle's render parameter to resolve #1566

merged 3 commits into from
May 29, 2021

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented May 27, 2021

I was a bit confused by the name render because it had a heavy page connotation in my mind (another user stated the same), so I wasn't sure if you'd be calling it for endpoints as well. Rich had suggested respond could be a more neutral alternative. I like that name, so wanted to raise the question of renaming it here

Copy link
Member

@ignatiusmb ignatiusmb left a comment

Choose a reason for hiding this comment

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

It runs on both pages and endpoints, so it describes the action better than render

@Conduitry
Copy link
Member

I dunno, respond doesn't feel right to me, and for what seems like a more serious reason than render. The function constructs the response, but nothing's returned from the server until you return a value from handle. This function in question renders the default response but doesn't respond with it.

@Rich-Harris
Copy link
Member

@benmccann suggested in discord that we change the signature to

export function respond({ request, handle }) {
  return handle(request);
}

I think calling the hook itself respond is fine. I'm not wild about handle as the argument name, it feels overly generic and suffers from the same problem as respond which is that it's a do-something verb rather than a get-something verb. The initial choice of render does have the correct connotation in that regard, though it seems people don't love that.

@benmccann
Copy link
Member Author

How about execute? Some other options: perform, fulfill, do, operate, serve, run, satisfy

@Rich-Harris
Copy link
Member

Those are all do-something verbs! They suggest that the function has an effect, rather than returning a value. A get-something verb is something like transform or map, though obviously neither works in this context. I still can't come up with something better than render

@elliott-with-the-longest-name-on-github
Copy link
Contributor

Maybe this is too far outside the box and I'm just not correctly conceptualizing the dataflow here, but Hooks in general really feel more like middleware to me than anything. Data flow is:

incoming request => handle
                         |_endpoints
                         |_getSession => anywhere session is used

As in, data flows into your application, through your hooks, and down to your loads/endpoints, which then (in the case of endpoints), respond to requests. The way I think about it, when I make a request to an endpoint from a .svelte file, I'm making a request through handle to the endpoint, which then responds to me.

If that's correct, the language around the hooks would make more sense if it sounded like a middleman:

export function handle({ request, continue }) {
  return continue(request);
}

// or 
export function handle({ request, forward }) {
  return forward(request);
}

I think that may shift the question to what handle should be called, though, as handle doesn't seem to fit that mental model as well. Maybe intercept?

@nitroboy720
Copy link

What about getResponse ?

@DhyeyMoliya
Copy link

DhyeyMoliya commented May 28, 2021

handle & renderer

Or

render and handler

Adding 'er' will make it more clear.

We are returning response handled by a handler.

Or we are returning response rendered by a renderer.

@quentin-fox
Copy link
Contributor

I think the handle name for the hook is slightly problematic since it doesn't clearly indicate that it's essentially a middleware function. It's also not really a traditional middleware, since it doesn't just have access to the request before it goes to the end route handler, but it has access to the handler itself.

Likewise, render seems much too front-end-y, and keeping the distinction between frontend and backend code in a framework where the two are located quite close to each other seems pretty important for clarity's sake.

With that in mind, thoughts on the name intercept for the hook, and instead of the render parameter, it could be changed to handler? So the snippet from the docs would look like:

export async function intercept({ request, handler }) {
	request.locals.user = await getUserInformation(request.headers.cookie);

	const response = await handler(request);

	return {
		...response,
		headers: {
			...response.headers,
			'x-custom-header': 'potato'
		}
	};
}

The idea of intercept or an interceptor is also in Nest.js where it is responsible for a similar roles as the handle hook, in that it is responsible for data flow before and after the end route handler is called.

Renaming render to handler also indicates that it's a route handler function that can be called with the request in order to obtain a response - I could be wrong, but I think that handler is a pretty conventional name for that sort of function.

@ignatiusmb
Copy link
Member

That's a nice reference, I like the idea of intercept more as this is sort of like a middleware but not really one, and to just say that this intercept(s) the request before returning a response, feels better somehow.

Still conflicted on the handler part.

@quentin-fox
Copy link
Contributor

quentin-fox commented May 29, 2021

How do you feel about process instead of handler? Servers tend to process requests, so the language kinda tracks, although once again it's just another do-verb.

EDIT: I take it back, if this runs on the server, that might shadow the process variable if someone does import process from 'process'.

@elliott-with-the-longest-name-on-github
Copy link
Contributor

Personally, I also like the idea of intercept. My vote for render is continue or forward:

export function intercept({ request, forward }) {
  return forward(request);
  // or return continue(request);
}

I'm flipflopping a bit on which I like better, but I think "continue" probably makes the most sense in my head.

@quentin-fox
Copy link
Contributor

quentin-fox commented May 29, 2021

I'm not so keen on forward, it doesn't really convey that you can receive the result of the request, kinda like next that's used in a lot of middleware patterns, which is just responsible for moving the request to the next handler. That train of thought is making me like continue a bit better as well.

@ignatiusmb
Copy link
Member

Please keep in mind of ES reserved keywords, words like do or continue can't be used.

@benmccann
Copy link
Member Author

After discussion with the other maintainers, I will rename to resolve

async function handle({ request, resolve }) {

}

@benmccann benmccann changed the title Rename handle's render parameter to respond Rename handle's render parameter to resolve May 29, 2021
benmccann and others added 2 commits May 29, 2021 14:09
remove reference to 'renderer', which is confusing now that we no longer use `render`
@Rich-Harris Rich-Harris merged commit eae1b1d into master May 29, 2021
@Rich-Harris Rich-Harris deleted the respond branch May 29, 2021 21:17
sidharthv96 added a commit to sidharthv96/kit that referenced this pull request May 30, 2021
…r/cutomExtensionTest

* 'master' of https://github.com/sveltejs/kit:
  fix: append trailing slash in manifest (sveltejs#1507)
  Version Packages (next) (sveltejs#1590)
  Rename `handle`'s `render` parameter to `resolve` (sveltejs#1566)
  Replace favicon (sveltejs#1589)
jthegedus added a commit to jthegedus/svelte-adapter-firebase that referenced this pull request May 30, 2021
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

Successfully merging this pull request may close these issues.

9 participants