-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
feat(nuxt): add reset to default functionality for useState and clearNuxtState
#33527
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
base: main
Are you sure you want to change the base?
Conversation
… management and reset functionality, introduce `_state` for internal use.
|
|
WalkthroughThe changes refactor Nuxt's state management system by introducing a new internal metadata storage mechanism ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes introduce new initialisation logic and internal state storage mechanisms in state.ts with modified public API signatures. Whilst the structural changes are localised to a single primary file and the logic is reasonably focused, the interplay between metadata storage, hydration, and reset behaviour requires careful reasoning. The addition of a new helper function and the introduction of the Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nuxt/src/app/composables/state.ts (1)
65-76: Safer key derivation and leaner payload clearing
_allKeysshould only include$s-prefixed entries; otherwise non‑useState keys (if any) would be transformed and re‑prefixed, causing unintended clears.- When removing without reset, prefer deleting the key to avoid retaining enumerable
undefinedslots (keeps payload slim and prevents stale keys from reappearing in subsequent clears).Apply:
- const _allKeys = Object.keys(nuxtApp.payload.state) - .map(key => key.substring(useStateKeyPrefix.length)) + const _allKeys = Object.keys(nuxtApp.payload.state) + .filter(key => key.startsWith(useStateKeyPrefix)) + .map(key => key.substring(useStateKeyPrefix.length)) for (const _key of _keys) { - clearNuxtStateByKey(nuxtApp, useStateKeyPrefix + _key, reset ?? true) + clearNuxtStateByKey(nuxtApp, useStateKeyPrefix + _key, reset ?? true) }And streamline per‑key clearing:
-function clearNuxtStateByKey (nuxtApp: NuxtApp, key: string, reset: boolean): void { - if (key in nuxtApp.payload.state) { - nuxtApp.payload.state[key] = undefined - } - - if (nuxtApp._state[key]) { - nuxtApp._state[key]!.data.value = reset ? unref(nuxtApp._state[key]!._default()) : undefined - } -} +function clearNuxtStateByKey (nuxtApp: NuxtApp, key: string, reset: boolean): void { + if (reset) { + if (nuxtApp._state[key]) { + // Resets both _state data and payload via toRef linkage + nuxtApp._state[key]!.data.value = unref(nuxtApp._state[key]!._default()) + return + } + // No _state registered: remove from payload if present + delete nuxtApp.payload.state[key] + } else { + // Remove from payload and null out internal value if tracked + delete nuxtApp.payload.state[key] + if (nuxtApp._state[key]) { + nuxtApp._state[key]!.data.value = undefined + } + } +}
🧹 Nitpick comments (2)
packages/nuxt/src/app/nuxt.ts (1)
145-151: Internal _state map looks good; consider GC of stale keysThe addition is well‑typed and initialised correctly. To avoid long‑lived growth on the client, consider pruning entries (e.g. delete nuxtApp._state[key]) when a state is cleared without reset and no watchers remain. This keeps memory proportional to active state keys.
Also applies to: 319-319
packages/nuxt/src/app/composables/state.ts (1)
45-56: You can simplify to a plain ref; computed indirection is unnecessaryGiven
_state[key]andstateare established above, returningstate as Ref<T>is sufficient. The computed fallback tonuxtApp.payload.state[key]is dead code in this path and adds overhead.Apply:
- return computed({ - get () { - return nuxtApp._state[key]?.data.value ?? nuxtApp.payload.state[key] - }, - set (value) { - if (nuxtApp._state[key]) { - nuxtApp._state[key]!.data.value = value - } else { - nuxtApp.payload.state[key] = value - } - }, - }) + return state as Ref<T>If you keep the computed, consider at least collapsing the branches to
state.valuefor both get/set. As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nuxt/src/app/composables/state.ts(2 hunks)packages/nuxt/src/app/nuxt.ts(2 hunks)test/nuxt/composables.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/app/nuxt.tstest/nuxt/composables.test.tspackages/nuxt/src/app/composables/state.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
test/nuxt/composables.test.ts
🧬 Code graph analysis (2)
test/nuxt/composables.test.ts (2)
packages/nuxt/src/app/composables/state.ts (2)
clearNuxtState(60-77)useState(20-57)packages/nuxt/src/app/nuxt.ts (1)
useNuxtApp(557-570)
packages/nuxt/src/app/composables/state.ts (1)
packages/nuxt/src/app/nuxt.ts (2)
useNuxtApp(557-570)NuxtApp(206-206)
🔇 Additional comments (1)
test/nuxt/composables.test.ts (1)
203-207: Test coverage aligns with intended semanticsThe scenarios comprehensively cover payload registration, unwrapped defaults, shared-key behaviour, and reset/remove paths. These rely on useState unref’ing the init return on initialisation; see my comment in state.ts proposing that fix.
Also applies to: 212-216, 217-224, 225-234, 243-285, 286-344
| if (state.value === undefined) { | ||
| nuxtApp.payload.state[key] = defaultFn() | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial value should unref the init return (prevents nested refs and matches reset path)
Currently defaultFn() is assigned directly, so passing () => ref(x) yields a ref-of-ref. Reset uses unref(...), so behaviours diverge. Unwrap on first assignment too.
Apply:
- if (state.value === undefined) {
- nuxtApp.payload.state[key] = defaultFn()
- }
+ if (state.value === undefined) {
+ // Unwrap to keep state a plain ref, even if init returns a ref/computed.
+ nuxtApp.payload.state[key] = unref(defaultFn())
+ }🤖 Prompt for AI Agents
In packages/nuxt/src/app/composables/state.ts around lines 41 to 44, the initial
assignment uses defaultFn() directly which can create a ref-of-ref when
defaultFn returns a ref; change the assignment to unwrap the value (e.g., use
unref(defaultFn()) or equivalent) so the first-assignment mirrors the reset path
and prevents nested refs; ensure unref is imported from Vue if not already.
CodSpeed Performance ReportMerging #33527 will not alter performanceComparing Summary
Footnotes |
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
|
Quick example addition: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it can be really confusing for users and quite hard to debug if there's multiple useState that retuns differents default values
I agree, E.g. You can tackle this by creating a composable around your I think it is a valid point about data storage and the keys that are being used, I've come across this myself while useing |
|
So what is next?
|
|
Yes that's just how it currently works with useAsyncData. |
| const useStateKeyPrefix = '$s' | ||
| const getDefault = () => undefined | ||
|
|
||
| type InitOption<T> = (() => T | Ref<T>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use MaybeRefOrGetter instead
|
|
||
| return computed({ | ||
| get () { | ||
| return nuxtApp._state[key]?.data.value ?? nuxtApp.payload.state[key] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't want to have two possible reference for a state 🤔
| if (key in nuxtApp.payload.state) { | ||
| nuxtApp.payload.state[key] = undefined | ||
| } | ||
| clearNuxtStateByKey(nuxtApp, useStateKeyPrefix + _key, reset ?? true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid breaking changes. We can make it true by default for v5 though by using a future flag
useStateandclearNuxtStateto improve statemanagement and reset functionality, introduce_statefor internal use.🔗 Linked issue
#32117
📚 Description
The setup for this PR is a starting point with a proposed solution
It does break the current logic as it does not automatically sets the state to undefined but resets it to the given
initfunctionOther paths considered:
initparameter aoptionsobject (fallback to oldinitparam),initvalueclearNuxtStateto keep current logic as is.nuxtApp._stateuseStateless clearuseAsyncDataTODO
When a solid path is determined, I will add guiding docs to the current functionality