feat(remix-server-runtime): allow generic types for `SessionData`

[cmd-johnson]

Currently, all session-related functions use the default SessionData interface that is defined as { [key: string]: any }. This works fine, but doesn't provide any type information for TypeScript when using e.g. session.get(...).

This PR adds generic types to

• the Session interface
• the createSession(...) function
• all built-in session stores

You can now create a session storage like this:

// app/sessions.ts
interface CustomSessionData {
  userId: number;
  error: string;
}

export const {
  getSession,
  commitSession,
  destroySession
} = createCookieSessionStorage<CustomSessionData>();

and use it like usual, but you will now get type information and auto-completion when using any of the session.(has|get|set|flash|unset) functions.

Using the generic session data type is optional and it defaults to the build-in SessionData interface, making this change fully backwards-compatible.

[ryanflorence]

Ooh, I like this, one problem though, the return of session.get needs to be <type> | undefined, you never know if the session actually has that data in it since it's stored in a browser cookie, a database, etc. That storage is invisible to the types system.

But the type system can force you to put the right kind of value in for set, and then tell you the type if it's actually defined after a get.

Make that change and we can merge.

For example:

let value = session.get("message");

This PR assumes value is a string, but it can't actually know that, it could be undefined. App code still needs to check at runtime if the value exists, and then TS can say it's a string:

let value = session.get("message");
// string | undefined
invariant(value, "expected message in session");
// string
console.log(value);

[cmd-johnson]

Hmm, i don't see it. The return type of Session<SD>.get(...) already is SD[K] | undefined: https://github.com/remix-run/remix/blob/98d6ad7831622866e0ba937960fca3925dc4a942/packages/remix-server-runtime/sessions.ts#L43

and when using it, everything looks correct on my side:

const { getSession } = createCookieSessionStorage<{ test: string }>()
getSession().then(s => {
  const x = s.get('test') // x: string | undefined
  const y = x.toUpperCase()
  // error: ^ Object is possibly 'undefined'
})

Or do you mean this in the way that while the type system can tell me that I get a string | undefined all it wants, the fact remains that the session storage could actually return a number, at which point all bets are off? In that case the return type of get() would have to be either any or (better) unknown, forcing you to verify that you actually got a string and not something entirely different. This sounds a bit impractical to me, though.

---

On a different note:

While implementing a login via OAuth2 in my Remix app today, I came up with one more possible addition: I noticed that everything I've stored in the session so far was either persistent (session.set(...)) or transient (session.flash(...)), never both. It could make sense to extend the interface further and introduce a second type parameter that is solely used for values that can be flash()ed. The second type parameter could default to the first (producing the same functionality as now), but you'd have the option to clarify which values you intend to be set() or flash()ed. get() would have to accept keys of either type parameter.

e.g.

export interface Session<SD = SessionData, FD = SD> {
  readonly id: string;
  readonly data: FlashSessionData<SD, FD>;
  has(name: (keyof SD | keyof FD) & string): boolean;
  get<K extends (keyof SD | keyof FD) & string>(name: K): (
    | (K extends keyof SD ? SD[K] : undefined)
    | (K extends keyof FD ? FD[K] : undefined)
    | undefined
  )
  set<K extends keyof SD & string>(name: K, value: SD[K]): void;
  flash<K extends keyof FD & string>(name: K, value: FD[K]): void;
  unset(name: (keyof SD & keyof FD) & string): void;
}

You could then do things like

interface SessionData { both: string, onlySet: 'set' }
interface FlashSessionData { both: number, onlyFlash: 'flash' }

const { getSession } = createCookieSessionStorage<SessionData, FlashSessionData>()
getSession().then(s => {
  s.get('both') // string | number | undefined
  s.get('onlySet') // 'set' | undefined
  s.get('onlyFlash') // 'flash' | undefined
  s.set('both', 'some string') // ok
  s.set('both', 10) // error: number is not assignable to string
  s.flash('both', 10) // ok
  s.flash('both', 'some string') // error: string is not assignable to number
  s.set('onlySet', 'set') // ok
  s.set('onlyFlash', 'flash') // error: 'onlyFlash' is not assignable to 'both' | 'onlySet'
  s.flash('onlyFlash', 'flash') // ok
  s.flash('onlySet', 'set') // error: 'onlySet' is not assignable to 'both' | 'onlyFlash'
})

What do you think? Should I add this to this PR as well?

[remix-cla-bot[bot]]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[cmd-johnson]

I noticed I missed adding the type parameters to the arcTableSessionStorage and fileStorage functions and amended those changes.

[cmd-johnson]

I went ahead and split up the types for get() and flash() data as I outlined above. Happy to revert the change if you think that goes a bit too far.

[ryanflorence]

Hmm, i don't see it.

Weird, I pulled down your code and was getting that it was just string all the time. I'll try again tomorrow and try to review the rest of your messages and code :)

[cmd-johnson]

@ryanflorence any chance of getting this merged? I'll gladly rebase these changes onto the current dev branch and resolve the conflicts then.

[MichaelDeBoey]

@cmd-johnson Could you please rebase your branch onto latest dev & resolve conflicts?

[cmd-johnson]

@MichaelDeBoey Done!

[JeffBeltran]

i would love to see this merged in, is there anything i could do to help with that?

[joseph-lozano]

the return of session.get needs to be <type> | undefined

Hi @ryanflorence

Is there a reason to not just return null if the key doesn't exist in the session?

I was actually bit pretty hard by this because Prisma treats undefined as "do nothing", while null is actually a value.

[kiliman]

I agree. I think Session.get() should return null if key not present. This will match FormData.get(). It helps when similar APIs work the same way.

https://developer.mozilla.org/en-US/docs/Web/API/FormData/get

[cmd-johnson]

Returning undefined is in line with how JavaScript's Map type works, though.

I think it also makes sense from a type perspective, where null means "I know that key/variable, but its value is empty" and undefined means "I don't know that key/variable". Always returning null would be throwing away type information that could come in handy.

There's use-cases for undefined (e.g. falling back to default values for parameters), that null wouldn't work with, and while you could work around that (fn(session.has("key") ? session.get("key") : undefined)), it's a lot more verbose. Returning undefined also lets you check if a key is present and getting the actual value with a single call.

If you really want to get null instead of undefined you could use session.get("key") ?? null or write a tiny wrapper around createSessionStorage(…) and make getSession(…) return sessions with a modified get function that does it for you.

[joseph-lozano]

@cmd-johnson, yeah it is in line with the Map type. changing it to null would be break backwards compatibility (at run time!), and is probably a no-go.

If I am not mistaken, the default type for SessionData is still any. Do you think it would be possible to switch that to unknown?

[cmd-johnson]

I'm usually all for using unknown over any, but this again would be a breaking change for anyone using the current session implementation without passing a custom SessionData type.

If you're concerned enough about types that you would like it to return unknown, you might as well pass a SessionData type and not deal with casting from unknown at all :wink:

[chaance]

Hi @cmd-johnson -- I'm looking to finally get this bad boy finalized and merged. Just a few tiny conflicts as a result of docs being moved around and updated names. Mind giving this one more rebase so I can knock it out this week?

[cmd-johnson]

I'll try and get this done this evening :+1:

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: d25491bb745bfb4cd0a914c362167d85afb4d959

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[github-actions[bot]]

🤖 Hello there,

We just published version v0.0.0-nightly-ba4dba9-20230210 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!