Bring over RR session code into @remix-run/session package #10797
Conversation
brophdawg11
force-pushed
the
brophdawg11/session
branch
from
003723b to
80537fe
Compare
brophdawg11
force-pushed
the
brophdawg11/session
branch
from
80537fe to
9dc0e74
Compare
brophdawg11
force-pushed
the
brophdawg11/session
branch
from
9dc0e74 to
d15afe1
Compare
| * Note: This function is typically not invoked directly by application code. | ||
| * Instead, use a `SessionStorage` object's `getSession` method. | ||
| */ | ||
| export function createSession<Data = SessionData, FlashData = Data>( |
There was a problem hiding this comment.
Seems like we could organize this code a little better by using a Session class. Then we wouldn't need a separate interface declaration up above. It would also make our isSession implementation simpler, or probably even redundant because people could just use obj instanceof Session instead.
We could still provide a createSession function for those who prefer doing things in a functional style, but internally it would just return new Session(...). I actually have a similar situation in the router where I recommend using createRouter in all the docs, but the router itself is implemented as a class.
| } | ||
|
|
||
| let response = await this.dispatch(request) | ||
| let context = await this.#parseRequest(request) |
There was a problem hiding this comment.
Good call here. The default handler should also have access to the fully parsed request context.
| // constructors can't be async | ||
| // - If we assign the session in the middleware, then `context.session` has | ||
| // to have a type of `Session | undefined` which is inconvenient for users | ||
| let session = await this.#sessionStorage.getSession(request.headers.get('Cookie')) |
There was a problem hiding this comment.
I see two options here:
a) Always use a root session middleware (as you're already doing in dispatch). In that case, we can get this code out of here and set context.session in the middleware. Then call next() to pass the request downstream and set the Set-Cookie header on the way back up.
b) Inline the logic from the session middleware in dispatch. This is essentially the approach we are already taking with the form data parsing and method override, both of which could be built as separate middlewares that always run.
The main difference is form data and method override need to affect matching, so they need to run before we know the full request context. The session doesn't affect route matching, but it is part of the request context so it feels similar to the other 2.
I think what I'd like to try is doing the session decoding here in this method, just below the creation of the context object. Keep the RequestContext constructor clean with a single request arg, and manually set context._session similar to how we're currently setting context.formData and context.method below. Then use a getter in context.session that returns this._session ??= new Session() to automatically create a new session when it is accessed if we didn't find one in the cookie.
Then, just before dispatch returns its response, it could automatically set the Set-Cookie header.
Ofc, none of this runs if they don't have a sessionStorage set. In that case, they'll just get a blank, new contex.session that won't do anything. This is similar to PHP where the $_SESSION superglobal is blank and doesn't do anything useful unless you call session_start() somewhere before you access it.
| this.#defaultHandler = options?.defaultHandler ?? noMatchHandler | ||
| this.#matcher = options?.matcher ?? new RegExpMatcher() | ||
| this.#parseFormData = options?.parseFormData ?? true | ||
| this.#sessionStorage = options?.sessionStorage ?? createCookieSessionStorage() |
There was a problem hiding this comment.
This should default to undefined meaning "I don't want to use sessions". We don't want to default to sending session cookies on every site, especially not in a GDPR world.
| "esbuild": "^0.25.10" | ||
| }, | ||
| "dependencies": { | ||
| "peerDependencies": { |
There was a problem hiding this comment.
I will take care of this on main so you don't need to do it in this PR. But thanks for bringing this to my attention.
| * An object of name/value pairs to be used in the session. | ||
| */ | ||
| export interface SessionData { | ||
| [name: string]: any |
There was a problem hiding this comment.
I think we should use unknown instead of any for the reasons explained in https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#any
| [name: string]: any | |
| [name: string]: unknown |
| /** | ||
| * Returns true if an object is a Remix session. | ||
| */ | ||
| export function isSession(object: any): object is Session { |
There was a problem hiding this comment.
| export function isSession(object: any): object is Session { | |
| export function isSession(object: unknown): object is Session { |
3 participants
Stacked on #10796
Adds a new
@remix-run/sessionpackage - brought over from https://github.com/remix-run/react-router/blob/main/packages/react-router/lib/server-runtime/sessions.ts