Skip to content

createFragment defeats its own reconcile identity contract #69

@nyanrus

Description

@nyanrus

Summary

createFragment's result observer pre-clears data to undefined inside the same batch() that subsequently writes the reconciled snapshot. Because solid-js/store's reconcile({ key, merge: true }) early-returns the new value as-is when the current store value isn't wrappable, reconciling against undefined produces a fresh top-level reference on every snapshot. The identity-preservation contract of reconcile({ key: "__id", merge: true }) is silently never honoured across snapshot updates.

Where

src/primitives/createFragment.ts, the resultUpdateObserver.next handler:

const resultUpdateObserver = {
  next(res) {
    queueMicrotask(() => {
      batch(() => {
        setResult("data", undefined);     // ← (1) clears data to undefined
        setResult("error", undefined);

        switch (res.state) {
          case "ok":
            setResult("error", undefined);
            setResult("pending", false);
            setResult("data", reconcile(   // ← (2) reconcile sees prev=undefined
              cleanSnapshot(res.value),
              { key: "__id", merge: true },
            ));
            break;
          case "error":
            setResult("data", undefined);
            setResult("error", res.error);
            setResult("pending", false);
            break;
        }
      });
    });
  },
} satisfies Observer<FragmentState<unknown>>;

Steps (1) and (2) happen inside one batch(), but setProperty on a Solid store applies its mutation synchronously — only the notification is deferred. By the time reconcile(...) runs in (2), the internal value at data is already undefined.

Why this defeats reconcile

solid-js/store's reconcile (modifiers.ts) opens with:

return state => {
  if (!isWrappable(state) || !isWrappable(v)) return v;
  const res = applyState(v, { [$ROOT]: state }, $ROOT, merge, key);
  return res === undefined ? (state as T) : res;
};

When state is undefined, the early return hands back the new value directly. Identity preservation by in-place merge only happens when both the previous and new values are wrappable. Pre-clearing to undefined guarantees the early-return branch every time.

Observable consequences

Identity-sensitive consumers re-mount on every Relay update, including pure field updates (reaction toggle, share count delta, polled cursor refresh). The most prominent of these is <Show keyed when={data()}>, which in solid-js's flow.ts re-runs its outer memo on every identity change of the condition value — the children function is invoked fresh on every snapshot, causing the entire subtree to be torn down and rebuilt.

Concrete trace, captured on https://hackers.pub/feed (Firefox profiler, 30 s recording with one composer open/close and one reaction toggle):

  • hackers.pub process heap: +10 MB/s sustained during interaction, vs. ~0 MB/s when idle on the same route.
  • Top inclusive samples: @kobalte/core/dist/chunk/* at ~1,200 of ~1,733 hackers.pub-thread samples.
  • Leaf-sample list dominated by _$createComponent — i.e., subtrees were being constructed continuously, not merely updated.

hackerspub enforces <Show keyed> for solid-relay-backed accessors via a custom Deno lint plugin (keyed-show) because non-keyed risks the documented "Stale read from <Show>" race when the fragment flips to null inside the same tick as a downstream reactive read. So the only way out for downstream consumers — short of patching solid-relay locally — is to give up the safety the lint plugin provides.

Inconsistency with createLazyLoadQuery

createLazyLoadQuery's subscriber in the same package handles only "ok" and "error" and does not pre-clear shared fields:

// src/primitives/createLazyLoadQuery.ts
next(state) {
  batch(() => {
    if (state.state === "ok") {
      setResult("error", undefined);
      setResult("pending", false);
      setResult("data", reconcile(cleanSnapshot(state.value), { key: "__id", merge: true }));
    } else if (state.state === "error") {
      setResult("data", undefined);
      setResult("error", state.error);
      setResult("pending", false);
    }
  });
},

So createFragment and createLazyLoadQuery already diverge on how they handle a "loading" state from relay-runtime:

  • createLazyLoadQuery: leaves data / error / pending untouched (stale-while-revalidate).
  • createFragment: clears data and error via the pre-clears at the top of its batch, then matches no case in the switch — effectively clearing the store on every "loading" tick.

This looks accidental (the createFragment switch has no "loading" case; the clear is a side effect of the unconditional pre-clears). Whichever behaviour is intended, it should be the same in both primitives.

Proposed direction

PR #68 restructures the observer so each branch owns its full state transition, no shared pre-clears, and "loading" is left untouched to match createLazyLoadQuery:

res.state data error pending
ok reconcile(value) undefined false
error undefined res.error false
loading unchanged unchanged unchanged

All updates remain inside the same batch(), so observers never see intermediate state — the fix is invisible to anything except identity comparisons across snapshot boundaries, which the reconcile contract is exactly there to keep stable.

If the maintainer prefers to preserve the current createFragment behaviour for "loading" (clear data and error) and align createLazyLoadQuery upward instead, the alternative is to add an explicit "loading" case in both primitives:

case "loading":
  setResult("data", undefined);
  setResult("error", undefined);
  break;

Either direction fixes the identity defeat described above; the choice between them is a UX call (flicker-to-empty vs. stale-while-revalidate) and worth being explicit about.

Reproduction

  1. Mount any <Show keyed when={data()}> over a createFragment accessor.
  2. Trigger any field update on the underlying record (e.g. a mutation that bumps an unrelated counter).
  3. Observe: the subtree under the Show is unmounted and remounted on every update. With heavy children (popovers, dropdown menus, hover cards), heap growth and CPU climb in proportion to update frequency.

A minimal reproduction can be constructed against any GraphQL schema with a mutable counter field; in production this manifests in any codebase that follows the <Show keyed> pattern recommended for solid-relay accessors.

AI disclosure

Investigation and proposed patch developed with Claude (Opus 4.7) as a programming assistant. The reasoning was verified by reading the relevant upstream source — solid-js/store's reconcile / applyState, solid-js's <Show> flow control, and relay-runtime's observeFragmentExperimental — and the references above point into that source for review.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions