Skip to content

fix(signals): guard node-less store staging reads - #4681

Closed
GabbeV wants to merge 1 commit into
solidjs:nextfrom
GabbeV:fix/nodeless-store-staged-screen
Closed

GabbeV wants to merge 1 commit into
solidjs:nextfrom
GabbeV:fix/nodeless-store-staged-screen

Conversation

@GabbeV

@GabbeV GabbeV commented Nov 4, 2026

Copy link
Copy Markdown

Summary

After #4625, reading latest(data) beside untrack(() => state.count) can throw TypeError: Cannot read properties of undefined (reading '_x') and halt reactivity when the store has no reactive node. The same program passes on #4625's direct parent.

stagedScreen accepts an optional signal, and the node-less store path deliberately omits it. Guard the new signal-specific holdStagingHook call when that argument is absent. This restores the previous node-less path while retaining #4625's hook for real signals, with no added state or allocation.

Single-button JSX repro, pinned to 2.0.0-rc.21. Click Run once: expected eventual 1 / 1; actual 0 / 0, the TypeError and REACTIVITY_HALTED. Removing only untrack is a passing control.

Adds render-effect and memo regressions, paired tracked-store controls, and a patch changeset. Each test checks the settled output and a second update.

How did you test this change?

  • On unchanged next@2105ffb8, both untracked regressions fail with the reported TypeError and both tracked controls pass. All four pass with the guard.
  • All 19 focused tests pass, including fix(signals): a held staging beats an inherited verdict posture, and a probing reader no longer blocks the landing (#4544) #4625's inherited-posture regressions and the existing store/latest read-after-write case. The new cases and fix(signals): a held staging beats an inherited verdict posture, and a probing reader no longer blocks the landing (#4544) #4625's tests also pass under production and observe defines (18 tests each).
  • Signals builds for all three tiers and source declarations pass: npm run build:js and npm run types in packages/signals.
  • Full signals suite after the build: 479 files / 6,506 tests pass, with 14 existing expected failures and two skips (vitest run --maxWorkers=2).
  • The actual JSX repro and tracked control pass in Chromium across production, development and observe builds: six browser cases.
  • Prettier, git diff --check, and node scripts/check-release-invariants.mjs pass. A separate test-tree TypeScript check reports the same 43 existing diagnostics on base and candidate, with no new diagnostics.
  • Rolldown 1.2.11: the isPending/latest scenario adds 3 minified bytes and is 1 Brotli byte smaller. The other three standard signals scenarios are unchanged. All four comparison gates pass; no caps changed.
  • Local Rolldown-bundled production timings on Node 24.19.0 covered chains, verdict reads and tracked store updates, with five warm-up batches and 11 alternating measured rounds. Results were noisy: the store median ranged from -0.8% to +9.0% across the initial runs. A fresh store-only process with 19 alternating rounds of 400,000 updates, and GC outside timed batches, measured +1.2%. The 9% result did not reproduce consistently; these timings are inconclusive for small performance effects.

@changeset-bot

changeset-bot Bot commented Nov 4, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 68be911

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
@solidjs/signals Patch
test-integration Patch
@solidjs/web Patch
@solidjs/babel-plugin Patch
@solidjs/compiler Patch
@solidjs/diagnostics Patch
@solidjs/element Patch
@solidjs/h Patch
@solidjs/html Patch
solid-js Patch
@solidjs/universal Patch
board-example Patch
canvas-example Patch
server-dbmon-example Patch
todos-server-example Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codspeed

codspeed Bot commented Nov 4, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

1 warning

⚠️ 1 benchmark is dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 201 untouched benchmarks
⏩ 5 skipped benchmarks1


Comparing GabbeV:fix/nodeless-store-staged-screen (68be911) with next (2105ffb)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@ryansolid

Copy link
Copy Markdown
Member

Thanks @GabbeV — great find and repro. Your fix landed as part of #4682, with your commit (68be911d) kept as-is. On top of it, we extended the guard so node-less store keys also get the #4544 rule: an inherited-posture pass is now served the hold's own write, the same as it would be from a signal or a key with a node. The guard alone served the committed value there, which tore the landing frame (posture-store-parity S9 pins it). Closing in favor of that PR.

@ryansolid ryansolid closed this Nov 4, 2026
@GabbeV
GabbeV deleted the fix/nodeless-store-staged-screen branch November 4, 2026 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants