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

docs: clarifying session/locals key difference #1457

Closed
wants to merge 1 commit into from
Closed

docs: clarifying session/locals key difference #1457

wants to merge 1 commit into from

Conversation

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

For newbies, wrapping your head around the data flow of SvelteKit takes a minute. Calling out specifically that locals don't have to be serializable like session does can clarify some of this (speaking from experience!).

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

Just proposing a one-line change to the documentation on request.locals. When I dove into SvelteKit for the first time, handle (then getContext) felt like it was making a HTTP request when it passed its response on to the endpoints. This gave me the impression that locals (then context) had to be serializable like session does. When I realized it didn't, it significantly simplified my auth/persistent session implementation. Calling this out clarifies this issue, especially for other newbies.

For newbies, wrapping your head around the data flow of SvelteKit takes a minute. Calling out specifically that locals don't have to be serializable like session does can clarify some of this (speaking from experience!).
@benmccann benmccann added the documentation Improvements or additions to documentation label May 16, 2021
@benmccann benmccann changed the title fix: Clarifying session/locals key difference docs: clarifying session/locals key difference May 18, 2021
@benmccann
Copy link
Member

I hadn't merged this because I wanted some other opinions on it. The more I think about it, the more I wonder if this is the right change. There's lots of stuff that runs just on the server and I'm not sure we'd want to put this disclaimer on all of them as it might become noisy. If there's a reason this call in particular is confusing I wonder if we can address the underlying issue instead. You said "handle felt like it was making a HTTP request when it passed its response on to the endpoints" and I wonder if there's some way we can address that

E.g. if we changed "This function runs on every request" to "This function runs on the server on every request" would that help?

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

Hey @benmccann!

I agree that we definitely wouldn't want to noise it up by adding this disclaimer to all of the code that runs server-side. I think that it's worth it in this special case, though.

The underlying problem is that all of the nomenclature around the request/endpoint setup "sounds" like HTTP. You make fetch requests to an endpoint you've set up in a .ts | .js file. Whether these are actual HTTP fetches depends on where they run -- client-side, obviously it's making a request back to your endpoint, which is then doing whatever it does. I'm assuming that when it runs server-side, this is not the case (though I'm not familiar with the internals on this). The whole handle => endpoint pipeline feels like HTTP because, well, it's supposed to (that's part of why it's so cool!). The locals object is even passed down on the request object, which itself is built to feel like a HTTP request.

This may not be the way to do it... but there should be some disclaimer or section here that shows how the handle => endpoint pipeline is not HTTP.

@benmccann
Copy link
Member

This isn't a special case though. And fetch is doing an HTTP call even on the server-side. I'm not quite sure why you think it isn't. You can fetch data from any URL like api.google.com or whatever, so there are times it has to be since it could be on a completely different server. But this has nothing to do with fetch or with making HTTP requests. handle is used for returning HTTP responses

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

@benmccann

I'm not sure I follow this:

But this has nothing to do with fetch or with making HTTP requests. handle is used for returning HTTP responses

How is handle for returning HTTP responses? I was under the impression it returned a ServerResponse, which was then passed to the endpoints. My whole overarching point being that it's pretty difficult to disambiguate what is HTTP (and therefore has to be serializable) and what isn't HTTP (and therefore does not), locals seeming to be a notable/important exception.

Addendum on why I assumed fetch might not be making an actual HTTP request on the server side: It seemed silly to make a HTTP request from the server to the server because it seems like it'd be slower than finding a way to "directly" call the endpoint on the server side when it's not going to a remote URL (api.google.com in your example). Good to know, though.

Little note: It's super hard to convey tone, but believe me -- I know you know the backend way better than I do, and I of course defer to you and the rest of the Svelte team's judgment. My opinion is just one of user experience.

@benmccann
Copy link
Member

A ServerResponse is a response from a server - hence it's an HTTP response. handle calls the endpoint - that's what render does. I do think the name render could be improved upon, so I'll open up a PR to suggest renaming it to respond

The name locals is supposed to denote that it's local to the application / server.

Thanks for taking the time to put together this PR. I'm going to go ahead and close it though because it seems like the confusion is at a more basic level and we should find a way to address that rather than this specific point

@benmccann benmccann closed this May 27, 2021
@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

@benmccann

Thanks for taking the time to review and converse!

I definitely agree that a change to the naming would do a better job of alleviating the confusion than adding to the docs. locals is a way better name than context was, and I think respond is definitely better than render. (The first time I saw render, I thought it had something to do with actually displaying a Svelte component, or something...).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants