fix(store): reconcile() notifies symbol-keyed properties (closes #2851)#2852
Open
yumemi-thomas wants to merge 2 commits into
Open
fix(store): reconcile() notifies symbol-keyed properties (closes #2851)#2852yumemi-thomas wants to merge 2 commits into
yumemi-thomas wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 023a932 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 |
Merging this PR will improve performance by 7.61%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | reconcile: deep tree, single deep() effect |
80.1 ms | 74.4 ms | +7.61% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing yumemi-thomas:fix/reconcile-symbol-keys (023a932) with next (070d853)
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 #2851
Summary
reconcile()never notifies subscribers of symbol-keyed store properties. An effect or binding trackingstate[SYM]doesn't re-run whenreconcile()changes that property, and because the stale node then shadows the value, even a plainstate[SYM]read keeps returning the old value — the reconciled value is unreachable. String-keyed properties on the same store update fine through the identical path, so the only difference is the key type.https://stackblitz.com/edit/solidjs-templates-krnhumcg?file=src%2FApp.tsx
This is the remaining variant of #2769: that fix (
bc92d002) covered symbol keys in the set trap,storeSetter,storePath, andmerge/omitvia theownEnumerableKeyshelper — butreconcile()'s own diff loops still enumerated withObject.keys, which drops symbols.Root cause
Every key enumeration in reconcile's diff dropped symbols:
getAllKeys(the$TRACK-tracked path) built fromgetKeys(string-only) +Object.keys(next).STORE_HAS(in-dependency) loops iteratedObject.keys(nodes).So
setSignalwas never called for symbol-keyed nodes, whileapplyState*had already swappedSTORE_VALUEto the new value — leaving the stale node shadowing it for direct reads too.Fix
Enumerate symbol keys everywhere reconcile enumerates string keys, while keeping the common symbol-free path on the exact
Object.keysfast path:getAllKeysbuilds string keys viaObject.keys(unchanged) and appends enumerable symbol keys viagetOwnPropertySymbols— symbol-free objects pay only an empty call, no per-key predicate.nodeKeys(new) enumerates a node record's keys:Object.keysalways, plus symbol nodes only for records flagged in asymbolKeyedRecordsWeakSet.getNodemarks a record when it creates a user (non-$TRACK) symbol node, and itsunobservedcleanup drops the mark once the last such node goes — so the flag means exactly "this record currently holds a user symbol node", and the reconcile hot path stays onObject.keysgated by a freeWeakSet.has.$TRACK(the internal self-node, the only internal symbol a node record can hold — the get/has traps early-return the others) is filtered out of symbol enumeration; callers handle it separately.Semantics are identical to the old code for string keys, and match
ownEnumerableKeys(enumerable strings + enumerable symbols) for the extended set; key order is irrelevant since each key is diffed independently.Tests
Seven tests in
tests/store/reconcile.test.ts(reconcile with symbol-keyed properties), the symbol cases confirmed failing red-first on the unfixed source:undefinedincheck updates when reconcile adds the keysymbolKeyedRecordsmark is set while tracked and cleared once unobserved (guards the fast-path optimization against a monotonic leak)Full solid-signals suite: the diff versus pristine is exactly the intended flips, nothing else; solid and @solidjs/web rebuilt against the fixed package and re-run at their baselines; source and test files pass the tests-inclusive typecheck.
Solid 1.x note
Exists in 1.x too, in a worse form: 1.x
reconciledoesn't even merge symbol-keyed values (the value stays stale; no notification question arises). 2.0 half-fixed this — the value merges — but subscribers of the symbol-keyed node are still never notified. Not a regression, but 2.0 already handles symbols in its other store paths (set trap,getAllKeys,merge/omitafter #2769), so finishing the job in reconcile's diff loops is consistent.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