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 into
mainfrom
fix/cookie-type
Nov 9, 2022
Merged

fix(nuxt): allow cookies to be set to null to unset them#8769
pi0 merged 5 commits into
mainfrom
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
@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

codesandbox Bot commented Nov 7, 2022

Copy link
Copy Markdown

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

@netlify

netlify Bot commented Nov 7, 2022

Copy link
Copy Markdown

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

Aareksio commented Nov 21, 2022

Copy link
Copy Markdown

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

Aareksio commented Nov 21, 2022

Copy link
Copy Markdown

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