Accept common cookie attributes when deleting a cookie

[fshafiee]

Changes

Allows cookie attributes to be specified when deleting a cookie. The attributes were limited to domain and path.

See #10480 for context.

Closes #10480

Testing

The changes are validated by unit tests.

Docs

The Astro.Cookie.delete API reference must be updated, since following statement will no longer be true:

Options allow setting the domain and path of the cookie to delete.

[changeset-bot]

🦋 Changeset detected

Latest commit: 28b353b

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

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

[lilnasy]

Looks great to me! Thanks for the clear tests as well.

Do you want to create a PR to the docs page as well? I can do it if not.

[ematipico]

I added this PR to the current milestone, so it will be shipped next release

[ematipico]

@fshafiee Can you open a PR against the docs repository and update the options? Let us know if you can or you can't, so we will make sure it will be ready in time for the next release.

[sarah11918]

Florian's changeset comment is good!

Also, just to make it easy, here's where the docs change needs to be made:

https://docs.astro.build/en/reference/api-reference/#delete

And all that needs to happen is something like this:

Marks the cookie as deleted. Once a cookie is deleted Astro.cookies.has() will return false and Astro.cookies.get() will return an AstroCookie with a value of undefined. Options available when deleting a cookie are: domain, path, httpOnly, sameSite, and secure.

[fshafiee]

Here’s the docs PR: withastro/docs#7814

CC: @sarah11918 @ematipico

[sarah11918]

Docs approves! 🥳

[jlarmstrongiv]

This commit broke Astro cookie types for me. Now, the types require all properties to be set, when previously they were optional. In this PR, see packages/astro/src/core/cookies/cookies.ts

[jlarmstrongiv]

For example:

error ts(2739): Type '{ httpOnly: boolean; maxAge: number; path: string; secure: boolean; }' is missing the following properties from type 'AstroCookieSetOptions': domain, expires, sameSite, encode
45   const defaultCookieOptions: AstroCookieSetOptions = {

[lilnasy]

@jlarmstrongiv It seems to be fine. Could you share the code snippet?

[nanek]

I also see the types for cookie options are all required now, as @jlarmstrongiv notes.
Running the sample @lilnasy provided above, throws the type error. Perhaps astro info is showing a different version that what is npm installed in that folder?

https://github.com/withastro/astro/pull/10671/files#diff-d3e7e8fe6551080102ade434b01c0baaf4f3be04289c1174294b114e466f21a0R5 is using Pick, which makes them required.

The tests are in javascript, so they don't pick up the type errors.

[lilnasy]

Thanks, I see it in a separate project, just not within the repo.

[fshafiee]

Pick and Omit don’t make optional fields required, unless using an ancient version of Typescript (<3.5.1). See microsoft/TypeScript#20722.

Here’s the proof:

[jlarmstrongiv]

@lilnasy here is the code snippet! Though it seems like you were able to reproduce it

@fshafiee here is proof that the types are definitely broken with typescript@5.4.5

https://github.com/jlarmstrongiv/astro-cookie-types

[ematipico]

Can anyone create an issue so we track it and fix it? Commenting a closed PR doesn't help the visibility

[fshafiee]

@jlarmstrongiv I understand that there’s an issue. I just meant to point out that it’s not due to the Pick syntax and the problem is probably somewhere else. 😅

I checked your repro and when I tried to follow the types, I realized that the cookie types are in fact unresolved, simply because they are missing from the dependencies (listed as devDependencies at the moment).

You can fix the issue by npm install @types/cookie, and you’d see the type-hints to appear, and the astro check will also pass.

@lilnasy @ematipico I've taken the liberty and opened a PR. Let’s continue the conversation there.