ReviewOS

stacks/stx

useFetch/useQuery: immediate:false leaves loading() stuck true, function URLs fetch twice on mount, and refetchOnFocus leaks a listener per instance

#1818
Closed glennmichael123 opened this 10 days ago · {{ comments.length }} {{ comments.length === 1 ? 'comment' : 'comments' }}
10 days ago

Three defects in the client runtime's data-fetching composables, found while planning a migration of 24 hand-rolled fetch() calls in a real app onto useFetch/useQuery. All three are in packages/stx/src/signals.ts and all three are independently verified against the source; two of them are visible in adjacent lines.


1. useFetch({ immediate: false }) leaves loading() stuck true forever

loading is initialised true unconditionally, but the only thing that clears it is the fetch that immediate: false suppresses:

function useFetch(urlOrFn, options = {}) {
  const data = state(options.initialData ?? null);
  const loading = state(true);            // <- unconditional
  const error = state(null);
  …
  // Auto-fetch on mount unless disabled
  if (options.immediate !== false) {
    onMount(fetchData);                   // <- the only thing that ever sets loading false
  }

So the documented way to declare a deferred request produces a composable whose loading() is true from creation until someone calls refetch() — which is precisely the case where a template is most likely to be driving a spinner off it. Any <div :if="loading()">Loading…</div> renders a permanent spinner for a request that was never issued.

loading should start false when immediate === false.

2. useFetch fires twice on mount when given a function URL

if (options.immediate !== false) {
  onMount(fetchData);              // request 1
}

if (typeof urlOrFn === 'function') {
  effect(() => {
    const url = urlOrFn();
    if (url && options.immediate !== false) {
      fetchData();                 // request 2 — effect() runs eagerly on creation
    }
  });
}

effect() runs its callback immediately when created, so a function URL with the default immediate issues two identical requests on mount — one from onMount, one from the effect's initial run. Doubling the request rate of every list/detail view that derives its URL from a signal, which is the intended use of the function form.

The effect should skip its first run (or onMount should be skipped when urlOrFn is a function).

There is a second-order consequence in the same block: because the refetch effect is gated on options.immediate !== false, a function URL combined with immediate: false never refetches when the URL changes either. The two options interact so that the function form is silently inert, which is unlikely to be intended — immediate should govern the first request, not whether the URL is reactive at all.

3. useQuery's refetchOnFocus listener is never removed

// refetchOnFocus
if (options.refetchOnFocus) {
  document.addEventListener('visibilitychange', function() {
    if (!document.hidden) fetchData();
  });
}

// refetchInterval
if (options.refetchInterval) {
  var intervalId = setInterval(fetchData, options.refetchInterval);
  onDestroy(function() { clearInterval(intervalId); });   // <- this one cleans up
}

The two blocks are adjacent and only one of them tears down. refetchOnFocus registers a document-level listener with an anonymous handler — so it cannot be removed after the fact even by the caller — and nothing ever removes it. grep -n visibilitychange packages/stx/src/signals.ts returns exactly one registration and no removeEventListener.

In an SPA this accumulates. Every navigation to a view that instantiates the query adds another permanent listener holding fetchData, its closure, and the whole query scope. Come back to a tab after visiting the same route five times and one focus event fires five requests. The retained closures are exactly the ones a developer would expect the scope teardown to release.

refetchInterval on the very next lines shows the intended shape:

var onFocus = function() { if (!document.hidden) fetchData(); };
document.addEventListener('visibilitychange', onFocus);
onDestroy(function() { document.removeEventListener('visibilitychange', onFocus); });

Note on onDestroy reachability

The suggested fix for (3) uses onDestroy, which is the same mechanism refetchInterval already relies on — so it is no worse than the status quo. But #1811 reports that the module-path onMount/onDestroy queues in signals-api.ts are never drained. If a query is ever constructed outside a lifecycle scope, onDestroy is a silent no-op and both the interval and the listener leak. Worth confirming the two are consistent when #1811 is resolved.

  • #1811 — onDestroy queues never drained on the module path.
  • #1805 — the browser-only composables that never reach the client; useFetch/useQuery are not in that set, they are live on window.stx (verified — 71/71 auto-imports resolve after #1804).
10 days ago

All three fixed in aebc6a43e3.

1. immediate:false stuck loading — starts false when nothing will be requested. Two existing tests pinned state(true) (the bug), so I updated them to the corrected contract rather than working around them.

2. Double fetch on a function URL — your diagnosis was exact: effect() runs eagerly, so its first run is the mount fetch. The onMount registration is now the else branch.

One subtlety worth recording: a naive fix breaks the reactive case. A first-run flag honours immediate:false for the initial evaluation only — a later change to the URL is a new request the caller asked for by changing it, not the initial one they deferred. There's a test for each half.

3. refetchOnFocus leak — now named and removed on destroy, matching refetchInterval three lines below. The handler being inline-anonymous is what made it a leak rather than an oversight: it was unremovable even if someone had tried.

That test is structural. Driving a full scope teardown through the test harness would exercise the teardown machinery rather than the pairing, and the pairing is the defect.

Sign in to comment on this issue.