Skip to content
This repository was 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
pi0 merged 5 commits intomainfrom
fix/cookie-type
Nov 9, 2022
Merged

fix(nuxt): allow cookies to be set to null to unset them#8769
pi0 merged 5 commits intomainfrom
fix/cookie-type

Conversation

@danielroe
Copy link
Copy Markdown
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
Copy Markdown

codesandbox Bot commented Nov 7, 2022

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

@netlify
Copy link
Copy Markdown

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
Copy Markdown

@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.

@danielroe
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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.

4 participants