Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

fix(nuxt): allow cookies to be set to null to unset them #8769

Merged
merged 5 commits into from Nov 9, 2022
Merged

Conversation

danielroe
Copy link
Member

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

remake of #8754

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

magyarb and others added 4 commits November 6, 2022 13:21
### Discussed in https://github.com/nuxt/framework/discussions/2576

The cookie.value is typed as string, so // @ts-ignore must be used to do this. I have fixed the types, so the cookie can be deleted without a ts error.
@danielroe danielroe added bug Something isn't working types πŸ”¨ p3-minor-bug Priority 3: a bug in an edge case that only affects very specific usage labels Nov 7, 2022
@danielroe danielroe requested a review from pi0 November 7, 2022 11:58
@danielroe danielroe self-assigned this Nov 7, 2022
@codesandbox
Copy link

codesandbox bot commented Nov 7, 2022

CodeSandbox logoCodeSandbox logoΒ  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@netlify
Copy link

netlify bot commented Nov 7, 2022

βœ… Deploy Preview for nuxt3-docs canceled.

Name Link
πŸ”¨ Latest commit 436d7f2
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/6368f358986c420009a2febd

@danielroe danielroe mentioned this pull request Nov 7, 2022
7 tasks
@pi0 pi0 changed the title fix(nuxt): allow cookies to be set to null to unset them fix(nuxt): allow cookies to be set to null to unset them Nov 9, 2022
@pi0 pi0 merged commit e76ebdd into main Nov 9, 2022
@pi0 pi0 deleted the fix/cookie-type branch November 9, 2022 08:59
@pi0 pi0 mentioned this pull request Nov 15, 2022
@Aareksio
Copy link

@danielroe This caused useCookie<Type[]>('name', { default: () => [] }) to be Ref<Type[] | null> which forces us to write null checks while in reality the type is strictly never null.

Copy link
Member Author

@Aareksio I appreciate it; the issue is that it can be null if you set it to null. You could cast it as Ref<Type[]> in your case.

@Aareksio
Copy link

Aareksio commented Nov 21, 2022

After this change, all code must be explicitly null-checked or as-patched.
I would argue that neither of the following is good DX:

const languages = useCookie<string[]>('languages', { default: () => ['en'] }) as Ref<string[]>
// or
for (const language of (languages as Ref<string[]>).value) {}

I understand the reasoning on why the change was made. It just seems to break more (important) use-cases than it fixes.

@danielroe
Copy link
Member Author

I wonder if we could allow overriding with generic - so by default it infers a type with null, but you can override by setting your own value? And if you need to delete you can add | null to the generic type?

@Aareksio
Copy link

Aareksio commented Nov 21, 2022

Before the change, you should be able to do useCookie<string | null>() and the type would be Ref<string | null>. Rather painless opt-in.


https://github.com/nuxt/framework/blob/main/packages/nuxt/src/app/composables/cookie.ts#L26
Here it could be T = string | null as default, allowing for easy manual override if necessary.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3.x bug Something isn't working πŸ”¨ p3-minor-bug Priority 3: a bug in an edge case that only affects very specific usage types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants