feat(remix-server-runtime): make `Cookie` & `CreateCookieFunction` more type-safe

[mikeybinns]

• [ ] Docs
• [ ] Tests

Testing Strategy:

I'm not really sure about how to automate testing this because it only adds an optional type. npm run lint and yarn test run with no issues with my code. I don't see a way to run a specific type check.

I can use Typescript Intellisense in VS Code to see that the type is correctly applied when it is specified using the generic parameter for createCookie and the type is set to any when it is not (current behaviour).

e.g.

let cookie = createCookie(); 
let variable = cookie.parse(); // variable type = any. Current behaviour. 

let cookie = createCookie<string>(); 
let variable = cookie.parse(); // variable type = string

let cookie = createCookie<{some_value: string}>(); 
let variable = cookie.parse(); // variable type = {some_value: string}

fix note: Adding type safety to the serialise function produced an error in the function. This condition will always return 'false' since the types 'Type' and 'string' have no overlap. I fixed it by adding a type assertion to the value to make it read as "any". I don't know why the error occurred, to me it looks like a shortcoming with Typescript rather than an actual bug, but I think this should be double checked.

[mikeybinns]

@MichaelDeBoey I don't know if I need to request a review after I resolve these changes or if you get notified anyway, let me know if it's spamming you and I'll stop 👍

[mikeybinns]

@MichaelDeBoey Should the serialise function also allow null? This way, you can serialise null as a form of deleting the cookie, because parsing the cookie would return null, same as if it didn't exist. I don't know if there's a better way to handle clearing / deleting a specific cookie. there's no Delete-Cookie header to do so. I can only think of setting an expires that's in the past, but that's not exactly clean.

[mikeybinns]

Sorry @MichaelDeBoey I missed your response to this, all makes sense 😄

[mikeybinns]

@MichaelDeBoey I have done the work to make sessions work with a default value of unknown. Am I okay to rebase the patch-5 branch onto the dev branch and then commit those changes?

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 532966f75f178f89c6f11eb7a45785cc9b0a0b07

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

[mikeybinns]

@MichaelDeBoey Is there anything I can do to get this PR merged? From my point of view it looks ready, but it hasn't been included in any of the releases since sign off and it's not in the Roadmap so I don't know this PR's status.

[mikeybinns]

@sergiodxa has raised a good point in this discussion that cookies are user input and therefore could be different from what we expect. This shouldn't be a problem with encrypted cookies, so I'm going to try to update this PR to only type the cookie if a secret value is set.

[mikeybinns]

I've tried to do this over the weekend but I'm not able to work out how to do it because the function is returned by the factory function, rather than being its own named function. Perhaps someone with more Typescript knowledge may have better luck?

[chaance]

createCookie<string>() isn't really type-safe, it's basically just a fake promise in this case. Even with encrypted cookies, you still want to validate the input somewhere on your server.

My suggestion is to create a wrapper for createCookie that handles runtime validation for you. And if you really want to skip runtime validation you're free to do so, I just don't think we want to encourage that behavior!

import {
  createCookie as remixCreateCookie,
  type CookieOptions,
  type CookieParseOptions,
} from "@remix-run/node";

interface ValidatedCookieValue {
  whatever: string;
}

export function createCookie(
  name: string,
  cookieOptions?: CookieOptions
) {
  let cookie = remixCreateCookie(name, cookieOptions);
  return {
    ...cookie,
    async parse(
      cookieHeader: string | null,
      parseOptions?: CookieParseOptions
    ): Promise<ValidatedCookieValue> {
      let parsed = await cookie.parse(cookieHeader, parseOptions);
      assertValidCookieValue(parsed);
      return parsed;
    },
  };
}

This could also be done in a more flexible way, say if parse was generic and accepted its own validation function through which you could infer the return type. But you should be able to achieve what you're after as long as you're validating input.

[mikeybinns]

Okay, thanks @chaance, @sergiodxa echoed the same concerns and you're both right, we should be validating this cookie and not relying on false types 👍

[ansavchenco]

Maybe i'm missing something but can the result of cookie.parse be anything other than string or null/undefined if the cookie is not there? Why generic or any?