-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix(nuxt): sync watch request in useAsyncData #33192
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
Conversation
|
|
1db54f5 to
1143e35
Compare
WalkthroughThe useAsyncData watcher was refactored to separate key changes from other watch sources: a key-specific watcher ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing touches
🧪 Generate unit tests
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 |
1143e35 to
c4a8481
Compare
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
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: 2
🧹 Nitpick comments (1)
test/nuxt/use-async-data.test.ts (1)
359-406: Prefer a deterministic await oversetTimeout(5)and use a plain array for history.
- Using timers can be flaky under load;
await flushPromises()(or two ticks) is more deterministic for a debounce(0).requestHistorydoesn’t need to be reactive; a plain array avoids needless reactivity.You could simplify with:
- const requestHistory = reactive([]) + const requestHistory: any[] = [] @@ - // We need to wait for the debounce 0 - await (new Promise(resolve => setTimeout(resolve, 5))) + // Wait for the debounce(0) queue to flush + await nextTick() + await flushPromises()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nuxt/src/app/composables/asyncData.ts(2 hunks)test/nuxt/use-async-data.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:
test/nuxt/use-async-data.test.tspackages/nuxt/src/app/composables/asyncData.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/use-async-data.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (2)
packages/nuxt/src/app/composables/asyncData.ts (2)
325-349: Watcher split is the right fix; double‑check keeping key watcher asflush: 'sync'.Separating the key watcher from the user
options.watchsources removes the synchronous triggering that caused the race. Keeping the key watchersyncpreserves current semantics for key changes, but please verify scenarios where the key is computed from the same reactive sources mutated by other watchers. In such cases, asynckey watcher can still pre-empt queued non-sync watchers and run aninitialexecute before those finish. If that shows up, considerflush: 'pre'here as well or gating execution with a microtask.Would you like me to add a focused spec that mutates both a computed key and a watched params object in the same tick to assert the desired ordering?
359-361: LGTM on disposal.Disposing both watchers and unregistering the current key prevents leaks. Looks good.
| onScopeDispose(() => { | ||
| unsubExecute() | ||
| unsubKeyWatcher() | ||
| unsubWatcher() | ||
| unregister(key.value) | ||
| }) | ||
| } |
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.
🛠️ Refactor suggestion
Make the watcher flush explicit (non‑sync) and widen watch option typing.
- Being explicit about the flush makes intent clear and resists regressions.
- The code and new test support passing a reactive object to
options.watch, butAsyncDataOptions['watch']is still typed asMultiWatchSources. That will reject objects in TS.
Apply:
-const unsubWatcher = options.watch
- ? watch(options.watch, () => {
- asyncData._execute({ cause: 'watch', dedupe: options.dedupe })
- })
- : () => {}
+const unsubWatcher = options.watch
+ ? watch(
+ options.watch as any,
+ () => { asyncData._execute({ cause: 'watch', dedupe: options.dedupe }) },
+ { flush: 'pre' }
+ )
+ : () => {}Additionally (outside this hunk), update the option type so passing a reactive object is valid:
- watch?: MultiWatchSources
+ // Accept either multiple sources or a single reactive object
+ watch?: MultiWatchSources | Record<string, any>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const unsubWatcher = options.watch | |
| ? watch(options.watch, () => { | |
| asyncData._execute({ cause: 'watch', dedupe: options.dedupe }) | |
| }) | |
| : () => {} | |
| const unsubWatcher = options.watch | |
| ? watch( | |
| options.watch as any, | |
| () => { asyncData._execute({ cause: 'watch', dedupe: options.dedupe }) }, | |
| { flush: 'pre' } | |
| ) | |
| : () => {} |
🤖 Prompt for AI Agents
In packages/nuxt/src/app/composables/asyncData.ts around lines 351-356, the
watcher should be created with an explicit non-sync flush and the options.watch
typing must be widened to accept reactive objects; update the watch call to pass
watch(options.watch, () => asyncData._execute({ cause: 'watch', dedupe:
options.dedupe }), { flush: 'post' }) (making flush explicit and non-sync), and
elsewhere broaden AsyncDataOptions['watch'] to include reactive object types
(e.g. accept WatchSource<Record<string, any>> or Record<string, any> in addition
to MultiWatchSources) so passing a reactive object is allowed by TypeScript and
the new test will compile.
CodSpeed Performance ReportMerging #33192 will not alter performanceComparing Summary
|
| const unsubWatcher = options.watch | ||
| ? watch(options.watch, () => { | ||
| asyncData._execute({ cause: 'watch', dedupe: options.dedupe }) | ||
| }) |
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.
maybe we should have something like watchOptions 🤔 ?
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.
It can be nice to have, though it might mess around if users send immediate or other options which were not accounted for
The default options should already account for 99.9% of the cases (because it's already always deep and so pretty much the only options to play with are once and flush) and I don't think pre post would change anything here as the watcher doesn't do any DOM manipulation
I would still leave it for another PR if needed as this is a patch, so I wouldn't want to introduce new options
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
♻️ Duplicate comments (1)
test/nuxt/use-async-data.test.ts (1)
360-360: Prior review concern resolved:promiseFnis now defined.This addresses the earlier runtime error reported in past reviews. Looks good.
🧹 Nitpick comments (4)
test/nuxt/use-async-data.test.ts (4)
379-381: Avoid real timer sleeps; flush reactively instead to prevent flakes.Replace the arbitrary
setTimeout(5)with deterministic flushes.- // We need to wait for the debounce 0 - await (new Promise(resolve => setTimeout(resolve, 5))) + // Wait for watchers and pending microtasks to flush deterministically + await nextTick() + await flushPromises()
383-383: Ensure the second re-fetch has fully flushed before asserting.Add
flushPromises()afternextTick()to make the test deterministic.- await nextTick() + await nextTick() + await flushPromises()
359-359: Minor wording: hyphenate “non-synchronous”.- it('should watch params deeply in a non synchronous way', async () => { + it('should watch params deeply in a non-synchronous way', async () => {
406-407: Strengthen the assertion with an explicit call-count check.This verifies exactly three executions: initial + two subsequent watched updates.
]) + expect(promiseFn).toHaveBeenCalledTimes(3)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/nuxt/use-async-data.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:
test/nuxt/use-async-data.test.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/use-async-data.test.ts
🧬 Code graph analysis (1)
test/nuxt/use-async-data.test.ts (1)
packages/nuxt/src/app/composables/asyncData.ts (1)
useAsyncData(184-381)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: code
Co-authored-by: Robin <[email protected]>
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
♻️ Duplicate comments (1)
test/nuxt/use-async-data.test.ts (1)
365-365: Fix TypeScript inference:requestHistoryisnever[].
const requestHistory = []infersnever[]under strict TS, makingpush(...)a type error. Type the array explicitly.- const requestHistory = [] + type RequestParamsSnapshot = { deep: { baz: string }, foo?: string, locale?: string } + const requestHistory: RequestParamsSnapshot[] = []
🧹 Nitpick comments (3)
test/nuxt/use-async-data.test.ts (3)
379-381: Make the test deterministic without real-time sleeps.Replace
setTimeout(5)with tick/flush to reliably batch updates and flush watchers.- // We need to wait for the debounce 0 - await (new Promise(resolve => setTimeout(resolve, 5))) + // Batch updates and let watchers/asyncData flush + await nextTick() + await flushPromises()
373-376: PreferstructuredCloneover JSON stringify for deep cloning (fallback-safe).This avoids pitfalls with non-JSON data and is clearer.
- await useAsyncData(uniqueKey, async () => { - requestHistory.push(JSON.parse(JSON.stringify(params))) + await useAsyncData(uniqueKey, async () => { + const snapshot = typeof structuredClone === 'function' + ? structuredClone(params) + : JSON.parse(JSON.stringify(params)) + requestHistory.push(snapshot) await promiseFn() }, { watch: params })
384-407: Strengthen assertion: verify fetch invocations count.Ensures no extra/missed executions slip in unnoticed.
expect(requestHistory).toEqual([ { deep: { baz: 'baz', }, foo: 'foo', locale: 'en', }, { deep: { baz: 'baz', }, foo: 'bar', locale: 'fr', }, { deep: { baz: 'bar', }, foo: 'bar', locale: 'fr', }, ]) + expect(promiseFn).toHaveBeenCalledTimes(3)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/nuxt/use-async-data.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:
test/nuxt/use-async-data.test.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/use-async-data.test.ts
🧬 Code graph analysis (1)
test/nuxt/use-async-data.test.ts (1)
packages/nuxt/src/app/composables/asyncData.ts (1)
useAsyncData(184-381)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: code
Co-authored-by: Robin <[email protected]>
🔗 Linked issue
Resolves #33189
📚 Description
This fixes an issue where the requests would execute before the watched parameters were updated by other watchers