fix(reconcile): unwrap store proxies to their raw value in the diff (closes #2825)#2826
Open
yumemi-thomas wants to merge 1 commit into
Open
fix(reconcile): unwrap store proxies to their raw value in the diff (closes #2825)#2826yumemi-thomas wants to merge 1 commit into
yumemi-thomas wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: cfe6c8f The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
79f4137 to
cfe6c8f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2825
Summary
reconcile()mishandles values that are store proxies — the normal shape for store-in-store composition ({ selected: row }, lists of row stores, whatcreateProjectioncommits). Once the nested proxies have live tracked nodes (i.e. they are rendered anywhere), three things go wrong: a keyed swap writes internal signal-node records into the store (bindings render[object Object],JSON.stringifythrows on circularity), a keyless swap is silently dropped, and a keyed reorder of an array whose items are proxies crashes synchronously withRangeError: Maximum call stack size exceeded. All three are 1.x → 2.0 regressions.Repro (one panel per symptom): https://stackblitz.com/edit/solidjs-templates-qjajl3gp?file=src%2FApp.tsx
Root cause
Two related gaps in
store/reconcile.ts:The diff's
unwraphelper reads the wrong slot off a proxy:value?.[$TARGET]?.[STORE_NODE]— the per-property signal-node bookkeeping record — instead ofSTORE_VALUE, the raw data. The object diff then compares, keys, and re-wraps the internal record (symptoms 1–2). It has been this way since the helper was introduced (880c97b3), likely a slip between the adjacent one-letter slots ("n"vs"v"); it survived because the wrong slot is only populated once nested proxies are rendered, which non-rendering tests never do. 1.x deep-unwraps proxies before diffing — this helper appears to be the port of that step, pointed at the wrong slot.Nothing normalizes
nexton entry: the array branches (and rootreconcile()calls) handapplyStatea proxy asnext, whose swap then sets a row store'sSTORE_VALUEto its own proxy — every subsequent read recurses through itself (symptom 3).Fix
Two lines:
unwrapreadsSTORE_VALUE, andapplyStatenormalizesnext = unwrap(next)as its first statement. All next-side routes into the diff (object props, every array branch, recursion, root calls) funnel through that single entry, and only the next side is ever written — the previous side is read-only and proxy-safe by construction (keyFn/keyedMatchread through proxies;wrap()short-circuits on$PROXY).unwrapdeliberately stays a shallow raw read rather than reusingunwrapStoreValue: that helper clones when aSTORE_OVERRIDEis pending, and clones break the diff's identity mechanics (previous === nextskip checks,wrap(raw)resolving back to the existing proxy viaSTORE_LOOKUP). Pending overrides are honored at the right layer instead —wrap(raw)returns the same proxy with its overrides, and the recursion dispatches to the override-aware slow path.Beyond fixing the crash, this restores reconcile's identity contract across the proxy boundary: after a keyed reorder
list[0] === rowBholds, and reconciling raw server data into a list of proxies deep-merges into the existing row stores (their independent subscribers update) instead of replacing them.Solid 1.x note
All three scenarios verified working on
solid-js@1.9.14: the keyed swap lands with identity (selected === rowB), the keyless swap applies, and the proxy-array reorder preserves identity on every row. Clean full regression, no caveats.Tests
Three red-first tests in
tests/store/reconcile.test.ts, one per symptom:Each confirmed failing on the unfixed source and reproducing against the published
solid-js@2.0.0-beta.15npm package; the crash case additionally asserts row identity is preserved through the reorder. Full solid-signals / solid-js / @solidjs/web suites pass.Everything before the 1.x note stays as given. The claims in the paragraph are all accurate for this PR — the three tests were red-first (verified on unfixed source), and both the bugs and the fixed behavior were run against published beta.15 (bugs) and the built branch (fixes). Delete the trailing --- + italic line from the previous version so the disclosure appears exactly once.
Full disclosure: I wrote this with the help of an AI assistant (Claude Fable 5) and reviewed every line before pushing. All new tests were written first and confirmed failing on the unfixed code, and the bugs and fixes were both verified against the published beta.15 npm package, not just this repo.