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

@supabase/ssr should include @types/cookie as a (non-dev) dependency #53

Closed
2 tasks done
dawaltconley opened this issue May 2, 2024 · 7 comments
Closed
2 tasks done
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@dawaltconley
Copy link

Bug report

  • I confirm this is a bug with Supabase, not with my own application.
  • I confirm I have searched the Docs, GitHub Discussions, and Discord.

Describe the bug

The server client example for NextJS setups throws errors with stricter TS/ESLint configs (specifically with the @typescript-eslint/no-unsafe-argument rule):

// from https://supabase.com/docs/guides/auth/server-side/nextjs
import { createServerClient, type CookieOptions } from '@supabase/ssr'
import { cookies } from 'next/headers'

export function createClient() {
  const cookieStore = cookies()

  return createServerClient(
    process.env.NEXT_PUBLIC_SUPABASE_URL!,
    process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!,
    {
      cookies: {
        get(name: string) {
          return cookieStore.get(name)?.value
        },
        set(name: string, value: string, options: CookieOptions) {
          try {
            // ERROR: Unsafe argument of type `any` assigned to a parameter of type `[key: string, value: string, cookie?: Partial<ResponseCookie> | undefined] | [options: ResponseCookie]`
            cookieStore.set({ name, value, ...options })
          } catch (error) {
            // The `set` method was called from a Server Component.
            // This can be ignored if you have middleware refreshing
            // user sessions.
          }
        },
        /* ... */
      },
    }
  )
}

It's not immediately obvious when trying to debug but { name, value, ...options } evaluates to any. This is because CookieOptions is an alias for a type provided by the @types/cookie package, but this is a devDependency, so it's not being installed with @supabase/ssr:

// from dist/index.d.ts
import { CookieSerializeOptions } from 'cookie';

type CookieOptions = Partial<CookieSerializeOptions>;

As is, CookieOptions is basically an obscured any type, which only makes the type issues harder to debug.

To Reproduce

Attempt to use the server client example in any project with @typescript-eslint/no-unsafe-argument set to error.

Expected behavior

This can be fixed by adding @types/cookie to a project manually, but it would be better to include the package with @supabase/ssr.

@dawaltconley dawaltconley added the bug Something isn't working label May 2, 2024
@encima encima transferred this issue from supabase/supabase May 3, 2024
@encima
Copy link
Member

encima commented May 3, 2024

Transferred to auth for the right eyes. PRs welcome as well!

@rbutera
Copy link

rbutera commented May 17, 2024

I've created a PR over at @supabase/auth-helpers:

supabase/auth-helpers#781

Thank you @dawaltconley for figuring this out as it was a bit of a head scratcher for me!

@ryangriffinau
Copy link

Had the same issue and could not work it out - thanks @dawaltconley !

@J0 J0 transferred this issue from supabase/auth Aug 27, 2024
@Simplereally
Copy link

Ran into this issue today and was banging my head against the wall for a while.

For those who are unsure of the solution:

  1. Install cookie type as dev dependency as OP mentioned via bun add @types/cookie -d or npm i -d @types/cookie
  2. Update server.ts to include:
import { type CookieSerializeOptions } from "cookie";
type CookieOptions = Partial<CookieSerializeOptions>;
  1. Use it in the set function:
cookiesToSet.forEach(({ name, value, options }) =>
    cookieStore.set(name, value, options as CookieOptions),
);

@J0
Copy link
Contributor

J0 commented Sep 4, 2024

Hey everyone,

Thanks for the feedback. I can relate to the issue of having to manually install the types package and potentially doing some head scratching if one does not install it. However, my understanding is that types generally live as dev dependencies by convention and listing it as a prod dependency instead would force users to opt in even if they do not need the types

Leaving it as a dev dependency provides more optionality and I'm currently in favour of that. I think we can look into better documenting this on the SSR guide

I'll also check in with the team to see what they think. In the meantime let us know if there are any questions/concerns or more arguments for/against inclusion.

Thanks

@dawaltconley
Copy link
Author

For what it's worth, I do not think the convention on this is very clear or well-established, but I've seen more recommendations for library authors to install any exposed type dependencies as non-dev than dev (see here and here). The most contentious case is @types/node, since this introduces globals and is often installed manually.

In my view, every Typescript project already ships types to all users via declaration files, whether they use them or not. Unless you go pure JavaScript, it's really a choice between shipping complete vs incomplete types to all users. I know it's "one more dependency," but type packages are very small and usually (as in this case) 0 deps.

But I can't claim that's a universal view. Including @types/cookie in the installation part of the docs would probably resolve the confusion for most users (though it should probably be under optionalDependencies in that case).

@J0
Copy link
Contributor

J0 commented Sep 10, 2024

Hey,

We've decided to include it. Thanks!

hf pushed a commit that referenced this issue Sep 10, 2024
Makes the `@types/cookies` dependency a regular one to fix common
compilation errors. See #53.
@J0 J0 closed this as completed Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants