fix(store): snapshot() and store materialization keep symbol-keyed props#2895
Open
yumemi-thomas wants to merge 1 commit into
Open
fix(store): snapshot() and store materialization keep symbol-keyed props#2895yumemi-thomas wants to merge 1 commit into
yumemi-thomas wants to merge 1 commit into
Conversation
after writes (closes solidjs#2894)
🦋 Changeset detectedLatest commit: 85be00c 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 not alter performance
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | merge |
267.7 µs | 369.5 µs | -27.55% |
| ⚡ | merge |
278 µs | 190.1 µs | +46.26% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing yumemi-thomas:fix/snapshot-symbol-keys (85be00c) with next (d823a80)
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 #2894
Summary
After any write to a store subtree,
snapshot()of it drops symbol-keyed properties — and writing that subtree into another store drops them too. Before the first write they survive by accident (the identity-return fast path hands back the original object), so the loss only appears once the object has an override, and initial tests against untouched records pass.This is the remaining materialization variant of #2769: that fix covered symbol keys in the set trap,
storeSetter,storePath, andmerge/omitviaownEnumerableKeys, and #2851 coveredreconcile()'s diff loops — but the internal materialization paths still enumerated withObject.keys.Root cause
getKeys()seeds its base key list fromObject.keys(source)(strings only) and drives:snapshotImpl—snapshot()/deep()of any written record iterate string keys into the eagerly-created copy, so symbol props never land; and on the no-override path, symbol-keyed store-in-store values are never visited, so they leak wrapped instead of unwrapping.unwrapStoreValue— the setter's store-to-store materialization, so copying a previously-written subtree into another store drops its symbol props.walkAffectsScope— a keylessaffects(store)never descends into symbol-keyed subtrees, so their nodes miss pending coverage.Note the boundary: symbols living in the override layer (added after creation) were already enumerated (
Reflect.ownKeys(override)in the merge loop) — the gap was specifically base-object symbols, which is why the failure needs a prior write to appear.Fix
New symbol-aware enumeration helpers, applied at exactly those three sites; the public
getKeysis unchanged, soreconcile'sgetAllKeysand theownKeystrap keep their existing paths:getStoreKeys— enumerable strings + enumerable symbols, honoring override adds and$DELETEDremovals. Plain objects stay on theObject.keysfast path and only pay onegetOwnPropertySymbolscall (ownEnumerableKeysPlain); wrapped store-in-store sources keep the single-passownEnumerableKeysso theirownKeystrap fires once, untracked.getStoreSymbols— symbol-only variant for the array branches, so large index lists aren't allocated and scanned a second time; both array walks (snapshot and affects) get a separate symbol pass after the numeric loop.lookupis set), sosnapshot()of a plain non-store object keeps its old string-only behavior. Non-enumerable symbols are filtered, so internal slots can never leak into copies (test-pinned). Symbol-keyed accessors are skipped, matching the string-key branches.Tests
Seventeen new tests (ten confirmed failing red-first on the unfixed source; seven are boundary-semantics controls):
tests/snapshot.test.ts— symbol keys survive after a write; a write inside a symbol-keyed subtree is captured; unread and written-parent symbol-keyed store-in-store values unwrap; array symbol metadata survives an index write; store-to-store materialization keeps symbols; shared references between symbol and string keys stay aliased in the copy; cycles through symbol keys preserve identity; deleted symbol keys (object and array) stay deleted; non-enumerable symbols stay excluded from copies; a symbol key added after a prior snapshot appears; untouched nested values keep symbolstests/store/utilities.test.ts—deep()tracks updates in a symbol-keyed subtree, and notifies on symbol key addition and deletiontests/question-scoped-pending.test.ts— keylessaffects(store)covers an untouched symbol-keyed grandchild, and symbol-keyed array metadataFull solid-signals suite: the diff versus pristine is exactly the intended flips, nothing else — including the #2893 affects-audit tests, which exercise the same subsystem as the
walkAffectsScopechange.solid-jsand@solidjs/webrebuilt against the fixed package and re-run at their baselines; source passes the typecheck. Verified atnext@d823a806(2.0.0-beta.19).Performance
Micro-benched with symbol-free fixtures (the common case): no measurable change for written arrays, store-to-store writes, wide objects (~3%), or the existing
reconcile/deep()benches. Snapshotting a deep tree of many small objects pays ~14% — onegetOwnPropertySymbolscall per visited object, which is the minimum price of seeing symbol keys at all.snapshot()is a serialization-boundary operation, not a per-read hot path. If this matters, a follow-up could gate enumeration behind aWeakSetmarker like #2851'ssymbolKeyedRecords.Does this exist in Solid 1.x?
Regression. Verified against 1.9.14:
unwrapafter a write keeps symbol keys, and copying a written subtree into another store keeps them.Full disclosure: I wrote this with the help of an AI assistant (Claude Fable 5) and reviewed every line before pushing. All red-first tests were confirmed failing on the unfixed code.