Skip to content

Conversation

@Tofandel
Copy link
Contributor

@Tofandel Tofandel commented Sep 10, 2025

🔗 Linked issue

Resolves #33189

📚 Description

This fixes an issue where the requests would execute before the watched parameters were updated by other watchers

@Tofandel Tofandel requested a review from danielroe as a code owner September 10, 2025 12:40
@bolt-new-by-stackblitz
Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@coderabbitai
Copy link

coderabbitai bot commented Sep 10, 2025

Walkthrough

The useAsyncData watcher was refactored to separate key changes from other watch sources: a key-specific watcher (unsubKeyWatcher) now handles unregistering/registering entries, dependency counts, and conditional initial fetches when the primary key changes; a second watcher (unsubWatcher) observes options.watch and triggers asyncData._execute({ cause: 'watch', dedupe }) on those source changes. Cleanup now disposes both watchers. A new test (test/nuxt/use-async-data.test.ts) was added to verify deep, non-synchronous watching of a reactive params object; that test appears duplicated in the diff.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks (4 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning The functional changes are limited to packages/nuxt/src/app/composables/asyncData.ts and related tests, which align with the linked issue; however the test file contains a duplicated test insertion (the same new test appears twice), which appears accidental and is unrelated to the fix itself. Remove the duplicated test entry from test/nuxt/use-async-data.test.ts and run the test suite/CI again to ensure only the intended test remains and there are no accidental regressions from the duplicate.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix(nuxt): sync watch request in useAsyncData" concisely identifies a bugfix in useAsyncData related to the watch/request synchronisation and reflects the primary intent of the changeset described in the PR and raw_summary. It is short, focused on the main change, and will be understandable to reviewers scanning history.
Linked Issues Check ✅ Passed The changes shown (splitting the single watcher into a dedicated key watcher and a separate watcher for options.watch, updating cleanup, and adding a test that verifies non-synchronous/debounced refetch behaviour for deep reactive params) directly address the root cause described in issue #33189 and the PR objectives to avoid starting requests before other watchers complete. The new test exercises the exact scenario from the issue, demonstrating the intended fix.
Description Check ✅ Passed The PR description clearly references the linked issue (#33189) and succinctly states the problem being fixed (requests executing before watched parameters were updated), which is directly related to the changes in asyncData.ts and the added tests; it is not off-topic.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 95946c9 and e9965d1.

📒 Files selected for processing (1)
  • test/nuxt/use-async-data.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • 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: build
  • GitHub Check: codeql (javascript-typescript)
  • GitHub Check: code
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Tofandel Tofandel force-pushed the fix/async-data-sync-watch branch from 1143e35 to c4a8481 Compare September 10, 2025 12:43
@pkg-pr-new
Copy link

pkg-pr-new bot commented Sep 10, 2025

Open in StackBlitz

@nuxt/kit

npm i https://pkg.pr.new/@nuxt/kit@33192

nuxt

npm i https://pkg.pr.new/nuxt@33192

@nuxt/rspack-builder

npm i https://pkg.pr.new/@nuxt/rspack-builder@33192

@nuxt/schema

npm i https://pkg.pr.new/@nuxt/schema@33192

@nuxt/vite-builder

npm i https://pkg.pr.new/@nuxt/vite-builder@33192

@nuxt/webpack-builder

npm i https://pkg.pr.new/@nuxt/webpack-builder@33192

commit: e9965d1

Copy link

@coderabbitai coderabbitai bot left a 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 over setTimeout(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).
  • requestHistory doesn’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

📥 Commits

Reviewing files that changed from the base of the PR and between b9ec650 and c4a8481.

📒 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.ts
  • packages/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 as flush: 'sync'.

Separating the key watcher from the user options.watch sources removes the synchronous triggering that caused the race. Keeping the key watcher sync preserves 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, a sync key watcher can still pre-empt queued non-sync watchers and run an initial execute before those finish. If that shows up, consider flush: '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.

Comment on lines +351 to +356
onScopeDispose(() => {
unsubExecute()
unsubKeyWatcher()
unsubWatcher()
unregister(key.value)
})
}
Copy link

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, but AsyncDataOptions['watch'] is still typed as MultiWatchSources. 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.

Suggested change
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-hq
Copy link

codspeed-hq bot commented Sep 10, 2025

CodSpeed Performance Report

Merging #33192 will not alter performance

Comparing Tofandel:fix/async-data-sync-watch (e9965d1) with main (3f7624b)

Summary

✅ 10 untouched

const unsubWatcher = options.watch
? watch(options.watch, () => {
asyncData._execute({ cause: 'watch', dedupe: options.dedupe })
})
Copy link
Member

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 🤔 ?

Copy link
Contributor Author

@Tofandel Tofandel Sep 10, 2025

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

@coderabbitai coderabbitai bot left a 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: promiseFn is 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() after nextTick() 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

📥 Commits

Reviewing files that changed from the base of the PR and between c4a8481 and d46ece4.

📒 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

Copy link

@coderabbitai coderabbitai bot left a 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: requestHistory is never[].

const requestHistory = [] infers never[] under strict TS, making push(...) 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: Prefer structuredClone over 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

📥 Commits

Reviewing files that changed from the base of the PR and between d46ece4 and 95946c9.

📒 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

@danielroe danielroe changed the title fix: sync watch request in useAsyncData fix(nuxt): sync watch request in useAsyncData Sep 10, 2025
@danielroe danielroe merged commit e2af8c6 into nuxt:main Sep 11, 2025
44 of 46 checks passed
@github-actions github-actions bot mentioned this pull request Sep 11, 2025
danielroe pushed a commit that referenced this pull request Sep 12, 2025
@github-actions github-actions bot mentioned this pull request Sep 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[useFetch/useAsyncData] Watched params breaks watcher/js expected synchronicity

3 participants