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 context to stuff #984

Closed
benmccann opened this issue Apr 12, 2021 · 27 comments · Fixed by #2439
Closed

Rename context to stuff #984

benmccann opened this issue Apr 12, 2021 · 27 comments · Fixed by #2439
Milestone

Comments

@benmccann
Copy link
Member

benmccann commented Apr 12, 2021

There are a couple different places we use the word context

  • load has a context parameter
  • getContext sets request.context

Adding this here to track and have a discussion at some point since a couple maintainers have mentioned this as being something we might want to considers

Also, it looks like perhaps we should fix this TODO in the process:

// TODO come up with better names for stuff

@benmccann benmccann added this to the 1.0 milestone Apr 12, 2021
@furudean
Copy link
Contributor

Yes please! I frequently see confusion on whygetContext() is not what is expected in a load() function. It probably doesn't help that the documentation on the matter is a bit sparse.

@rmunn
Copy link
Contributor

rmunn commented Apr 16, 2021

Should one be called serverContext and one be called clientContext? Except that load runs on the server and the client, so that's no good. How about endpointContext vs loadContext? Or just leave it called context when it's the thing returned by getContext, but call it loadContext in the load functions? I'm not strongly in favor of any of these, I'm just throwing out ideas here.

@Rich-Harris
Copy link
Member

I'm less concerned about the difference between context in load and getContext than I am about the overlap between context in load and request.context. If we could rename the latter, I think this would be less of an issue.

My previous suggestion was request.metadata/getMetadata(...) but people didn't love it.

getRequestInfo(...) -> request.info?

getRequestState(...)/getState(...) -> request.state?

@benmccann
Copy link
Member Author

getState(...) -> request.state sounds good to me

@Leftium
Copy link

Leftium commented Apr 30, 2021

The overloaded use of context definitely made SvelteKit more confusing and difficult for me to learn. Documenting what I see to be sources/inspiration for this conflict:


I propose changing load({ context }) to load({ info }):

  • Keep getContext() -> request.context because of deep parallels with the context in serverless lambda functions. (AWS/Netlify/etc...)
  • Many React context API tutorials often refer to managing "global component state". (React may have avoided using state because it already has a special meaning.)
  • However "state" seems to imply changes will be persisted/propagated back. Especially for those coming from React. So I prefer "info."
  • load({ componentContext }) is more descriptive, but results in a long variable name. It's really hard to find a term that describes one-way data-passing like "context"!

While we're discussing confusing naming, I think the "get" in getContext() and getSession() are named from the wrong perspective/direction:

  • Before I grokked how these hooks work together, I thought getContext() would be a method for getting information automatically generated by the server/SvelteKit runtime for the app/developer.
  • However, it's the place where apps/developers set information to be consumed later.
  • Even the documentation says "[getContext] generates..." So generateContext() would make more sense.
  • How about createContext() or setContext()?

@Leftium
Copy link

Leftium commented May 1, 2021

I just remembered the vanilla Svelte context API exists and already defines a getContext(). (Used in conjunction with setContext() for sharing data with child components.)

  • So that's a strong argument for changing getContext()/request.context.
  • Is there any reason SvelteKit doesn't simply use the existing Svelte context API? It's a little more verbose, but I think it's conceptually easier to understand.

I tried mapping out the different flow of information for the two different contexts:

image

@Conduitry
Copy link
Member

When context is getting passed down from async load to async load, none of the components exist yet, and so we can't use Svelte's concept of context.

@Leftium
Copy link

Leftium commented May 1, 2021

I figured out how to replace request.context with a more meaningful name:

  1. getContext()->request.context becomes addSession()->request.session (Add property request.session instead of request.context.)
  2. getSession() is renamed to websafeSession(session), and keeps protecting sensitive info from load({session}).

Bonus: easier to see the connection between request.session and load({session}).

Optional enhancements:

  • The default implementation of websafeSession(session) automatically passes request.session.WEBSAFE_* properties to load({session}) (Like the convention for VITE_ environment variables).
  • getContext() becomes adjustRequest(Request) => Request making it possible to do other things besides just adding a session property.

@DhyeyMoliya
Copy link

DhyeyMoliya commented May 1, 2021

One more idea :

  1. Hook getContext() renamed to getServerContext()
  2. Hook getSession() renamed to getClientSession()
  3. Server-side (Hooks, Endpoints) context renamed to serverContext
  4. Client session stays session
  5. Client context in load stays context

@Rich-Harris
Copy link
Member

The core team just had a long conversation about this and we like request.locals instead of request.context. Rather than having a getLocals(request) function we can do request.locals = {} before we pass it into handle:

function handle({ request, render }) {
  request.locals.user = get_user(request);
  return render(request);
}

function getSession(request) {
  return {
    user: sanitize_user(request.locals.user)
  };
}

We also spent entirely too long trying to come up with an alternative to layout context, but couldn't come to agreement. Some ideas that were floated, in no particular order of terribleness:

  • inherit
  • inherits
  • inherited
  • pipe
  • chain
  • context (keep as is)
  • locals (again)
  • tunnel
  • metadata
  • env
  • scene
  • domain
  • foo
  • $layout

@Rich-Harris
Copy link
Member

  • payload
  • bag
  • obj
  • _

@pngwn
Copy link
Member

pngwn commented May 1, 2021

As an experiment, for each API we wish to name. Could someone write a single sentence to describe them? Only one sentence is allowed.

@Rich-Harris
Copy link
Member

The more I think about it the more I lean towards either inherit or inherits

<!-- src/routes/$layout.svelte -->
<script context="module">
  import { createApiClient } from 'some-library';

  export async function load({ fetch }) {
    return {
      inherits: {
        api: createApiClient(fetch)
      }
    };
  }
</script>
<!-- src/routes/somewhere/else.svelte -->
<script context="module">
  export async function load({ inherits }) {
    const { api } = inherits;
    const banana = await api.get('banana');
    return {
      props: { banana }
    };
  }
</script>

I should at this point note that the above code will have a subtle client-side bug once we implement #1277, in that the fetch wrapper is constructed per-load-function-per-navigation. For it to work as advertised (i.e. invalidations trigger load granularly, rather than triggering all load functions) we'd need to wrap fetch once per app and track which load is currently running. This would all a fair bit of complexity however for something that shouldn't actually be user-observable, so maybe it's not an issue. (And it's a side issue to what we name stuff in any case.)

@pngwn
Copy link
Member

pngwn commented May 1, 2021

inherit doesn’t make sense from the parent because it is doing the opposite. And it doesn’t make sense from the child because inherit is a verb where as this is just a thing that exists rather than action to be performed. Conceptually it is an action but in terms of the js language semantics it just exists as a variable.

Why not call it provider/ provide / provision / supply or something? The layout provides some stuff and the child makes use of that provision.

@pngwn
Copy link
Member

pngwn commented May 1, 2021

yield, present, furnish

@Rich-Harris
Copy link
Member

Our thinking was that the name should be the same in both places — we don't want a situation like

<script context="module">
  export async function load({ input }) {
    return { output: {...} };
  }
</script>

provider, yield etc make sense as output but not so much as input. inherits isn't ideal but it sort of works if you squint. You can also use it as a noun — 'these are my inherits'

As an experiment, for each API we wish to name. Could someone write a single sentence to describe them? Only one sentence is allowed.

  • hooks — The developer is able to attach values to a request that can be used in endpoints and to derive session
  • load — The developer is able to pass values from layout components to page components (or intermediate layouts) via the load function

@Leftium
Copy link

Leftium commented May 1, 2021

If request.locals solves SvelteKit's context overlap confusion, what is the reasoning for also renaming layout load({context})? I think layout context makes sense and is similar enough to how it is used in other places:

  • Similar purpose of sharing data between components like Svelte/React context API
  • Similar read-only nature like context passed to serverless lambda functions (Netlify, AWS lambdas).

After keeping context, I like load({values}) because it confers the read-only nature even better than context. layoutValues is a little longer, but conveys returning it from a page's load() is a NOP:

  • hooks — The developer is able to attach values to a request that can be used in endpoints and to derive session
  • load — The developer is able to pass values from layout components to page components (or intermediate layouts) via the load function

I think any of these would work well:

  • request.values and load({context})
  • request.context and load({values})
  • (Also happy with request.locals)

@benmccann
Copy link
Member Author

I think we can make it clear that the two are linked without the name being exactly the same. E.g. if the layout output used provides and the component input used provided I think that's pretty clear

@pngwn
Copy link
Member

pngwn commented May 3, 2021

I think using the same name does make sense because it feels like the same object. Like a train passing through multiple stations. We don't use a setter to set the context or a getter to get. Rather we set up an object and pass that through as an argument. Implementation may differ and the semantics aren't crystal clear but this i my mental model for it.

@Rich-Harris While inherits might be technically correct, in popular usage I don't think it is common as a noun, especially not for non-english speakers. I think yield makes at least as much sense as inherits. We yield the values to children and we take from the yield. Alternatively, we add to the yield in a parent and take from it later in the lifecycle. It isn't perfect, though, I agree.

@arxpoetica
Copy link
Member

I was about to +1 yield, but can't use it:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/yield

Need a word that can be destructured and not also reserved.

I'm still pulling for pontoon.

@dummdidumm
Copy link
Member

Since context just fits best if it wasn't taken already, I propose layoutContext or loadContext to show a) the semantic similarity to contexts but b) make it visible that this is different and belongs to a specific part. The drawback of a few more keystrokes is a hit I'll gladly take.

@snoriq
Copy link

snoriq commented May 21, 2021

Recent update of load / handle / getSession works great & clear, thanks.

I have 2 questions or suggestions yet:

  1. can't we call (& trigger execution of) getSession() from the client?
    My particular need is that when I login through the client with firebase.auth, getSession() is not executed until I press a hard refresh of my site to trigger a SSR (and of course I have in hook/js/handle() some admin.firebase logic where I populate locals with user data for the session).
    One could object that authentification data are available through the firebase client api, but a) I'd like to keep it simple & rely on the server side to get some information, not a mix of client/server depending on the situation, b) for customClaims, the client-side api of firebase provides too much information (all claims, and not specificly the custom claims)

  2. wouldn't it make sense to have an option to lock the $session.user (or actually any particular session field or event $session) from any client update (in the svelte.config.js file for example...)?
    I know client changes of $session are not permanent and that $session is renewed at each SSR by getSession(), but my concern is about the safety of $session data. They are secure to use in load(), since originating from the server, but after the "module" load() phase, $session.user data should be considered as potentially corrupted (right ?).

Feedback welcomed; I must admit this is my first time ever making a web app/site... ^^

@georgecrawford
Copy link
Contributor

inheritance isn’t perfect, but it at least improves upon the verb/noun issue, which I completely agree should be a blocker as it’s very confusing.

Synonyms include bequest and primogeniture, which are both wonderfully ridiculous suggestions 😉

@georgecrawford
Copy link
Contributor

provision or provided could work?

@ignatiusmb
Copy link
Member

Loading in, receiving, and/or passing cargo

<script context="module">
  export const load = async ({ cargo }) => {
    return { cargo: { ... } };
  }
</script>

@rmunn
Copy link
Contributor

rmunn commented Sep 7, 2021

@snoriq wrote:

I have 2 questions or suggestions yet:

  1. can't we call (& trigger execution of) getSession() from the client?

You'll probably want to take a look at #1726 where I have a proposal that would allow that: I would add a $session.refresh() method on the session store (not the session argument to load(), but the store) which would call getSession() and load its result into the session store. (My proposal also adds a refreshSession() hook, and getSession() is a fallback if there's no refreshSession() hook.) I would welcome feedback on my proposal, if there's anything I missed.

My particular need is that when I login through the client with firebase.auth, getSession() is not executed until I press a hard refresh of my site to trigger a SSR (and of course I have in hook/js/handle() some admin.firebase logic where I populate locals with user data for the session).

With my proposal, that could be handled by having the login code call $session.refresh() once the Firebase auth code completes. Updating the $session store would then trigger any subscriptions you have on it, e.g. if you have a UserAvatar component that uses $session.user it would be auto-updated at that point.

Please let me know what you think, preferably over in #1726 so I have fewer issue comment threads to watch. :-)

@benmccann
Copy link
Member Author

The decision has been made to rename load's context to stuff. This idea courtesy of https://twitter.com/KentonVarda/status/1425622606821142530

@sveltejs sveltejs locked as resolved and limited conversation to collaborators Sep 14, 2021
@benmccann benmccann changed the title Possible renaming of some instances of context Rename context to stuff Sep 14, 2021
dummdidumm pushed a commit that referenced this issue Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.