Skip to content

fix(signals): treat a plain merge() result as an ordinary source (#3384) - #3401

Merged
ryansolid merged 1 commit into
nextfrom
fix/merge-stale-sources
Sep 13, 2026
Merged

ryansolid merged 1 commit into
nextfrom
fix/merge-stale-sources

Conversation

@ryansolid

Copy link
Copy Markdown
Member

Fixes #3384.

Problem

merge()'s plain-object form recorded its flattened sources on the result under the $SOURCES symbol, and a later merge() flattened through them instead of reading the object. But that result is a real object callers copy or mutate afterwards:

  • descriptor copies via Reflect.ownKeys carry the symbol along (this is what @yak/solid PR What about to use `lastChild` and `previousSibling` as well? #644's copyProps does, so $-prefixed props leaked onto DOM elements and the computed class was lost whenever the target spread {...props});
  • {...props} copies carry it too (enumerable symbol);
  • @solidjs/html (tagged-jsx.ts) calls mergeProps(props, spread) and then assigns later props and a children getter onto the result before createComponent — a component's own merge(defaults, props) then dropped them.

No cheap check validates the recorded sources (a same-key value overwrite is invisible to key counts or owner identity), so the plain form can't be trusted at all.

Fix

The plain-object result records nothing and is treated as an ordinary source by a nested merge. Only merge proxies, whose writes are no-ops, are still flattened through — that is also all mergeSources() (used by spread()) ever returned, so no consumer changes. omit() proxies keep answering $SOURCES with undefined (#3014).

Side effect worth knowing: merge(defaults, props) where props is a plain merge result covering every default now returns props directly — flattening used to defeat the "all keys covered → return the source" shortcut, so that common component pattern always allocated before.

Tests

Four new tests in utilities.test.ts; three fail on next:

  • descriptor copy of a merged object (the issue repro) — copy is read, $active gone, class present;
  • {...props} copy with a deleted and an overridden key;
  • html-style post-merge mutation (assign, add, defineProperty children);
  • guard: merge proxies still flatten (mergeSources(outer) is the flat list), plain results yield undefined.

Suites: signals 1769, solid 595, html 200, web dom 735 / server 790 / hydrate 169 — all green.

Performance

Interleaved A/B, old vs new dist/prod in one process, 41 alternating rounds, ratio new/old:

path ratio
merge(defaults, props5) construct 0.97
merge(props12, {children}) construct 0.96
nested merge(inner, {c}), not covered 1.05–1.10
nested merge(defaults, inner), covered 0.52
getter reads through 1 / 2 / 3 levels 0.84 / 1.01 / 1.06
proxy construct / read, omit 0.96–1.03 (unchanged path)

The one regression is the non-covered nested plain merge: it walks the intermediate result (bound getters, re-bound once more) instead of the original sources. The covered case, which is the merge(defaults, props) shape, no longer allocates at all.

merge()'s plain-object form stamped its flattened sources on the result
under the $SOURCES symbol, and a later merge() flattened through them
instead of reading the object. The result is a real object callers copy
(Reflect.ownKeys descriptor copies, {...props}) or mutate afterwards
(@solidjs/html assigns props and a children getter after spreading), so
the re-merge resurrected removed keys and dropped added or overwritten
ones. Nothing short of re-walking the object can validate the sources,
so the plain form now records none and is read like any other source;
only merge proxies, whose writes are no-ops, are still flattened.

Side effect: merge(defaults, props) with props a plain merge result that
covers every default now returns props instead of always allocating —
flattening used to defeat the covered-source shortcut.

Interleaved A/B (old vs new dist, same process): plain construct -3%,
nested-not-covered construct +5–10%, nested-covered construct -48%,
reads and proxy paths within noise.

Co-authored-by: Cursor <cursoragent@cursor.com>
@changeset-bot

changeset-bot Bot commented Sep 13, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9692c81

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

This PR includes changesets to release 11 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

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

@coveralls

coveralls commented Sep 13, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 34748640945

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage remained the same at 71.842%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 1007
Covered Lines: 772
Line Coverage: 76.66%
Relevant Branches: 790
Covered Branches: 519
Branch Coverage: 65.7%
Branches in Coverage %: Yes
Coverage Strength: 15.02 hits per line

💛 - Coveralls

@codspeed

codspeed Bot commented Sep 13, 2026

Copy link
Copy Markdown

Merging this PR will regress 4 benchmarks

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 33 improved benchmarks
❌ 4 regressed benchmarks
✅ 123 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
merge 264.5 µs 330.4 µs -19.93%
merge 227.2 µs 267.3 µs -15.02%
merge 265 µs 287.2 µs -7.73%
merge 74 µs 78 µs -5.18%
projection derive: write one NESTED field (reference) 830.9 µs 222 µs ×3.7
merge 45 µs 31 µs +44.93%
construct 56.5 µs 39.2 µs +44.23%
merge 46.6 µs 32.4 µs +43.66%
construct 56.6 µs 39.6 µs +42.96%
merge 45.4 µs 31.8 µs +42.72%
construct 56.7 µs 39.7 µs +42.6%
construct 58.6 µs 41.9 µs +39.85%
merge 49.2 µs 35.6 µs +38.25%
merge 48.6 µs 35.2 µs +37.99%
merge 67.6 µs 54.4 µs +24.27%
merge 68 µs 54.8 µs +24.06%
merge 72.2 µs 60.4 µs +19.54%
merge 55.1 µs 46.4 µs +18.85%
merge 54.3 µs 45.7 µs +18.71%
merge 87.6 µs 74.4 µs +17.73%
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing fix/merge-stale-sources (9692c81) with next (042b540)

Open in CodSpeed

@ryansolid
ryansolid force-pushed the fix/merge-stale-sources branch from 7b50875 to 9692c81 Compare September 13, 2026 08:53
@ryansolid

Copy link
Copy Markdown
Member Author

Reset the branch to 9692c81a (the original fix), dropping the two getter-record commits (fded3b2d, 7b508752).

Local utilities.bench.ts (merge cases, mean time vs next): v1 geomean 1.004× — faster on small merges (0.85–0.94×), slower only on wide, deep re-merges (deep 0,100,3,2, covered 100,25,5). The record variants moved that cost onto every plain result carrying a getter: geomean 1.16×, slower than v1 on 20 of 24 cases, and none of the deep cases recovered to next. The remaining CodSpeed regressions on this PR are the inherent price of not trusting a plain result's origins; a design that makes nested merges cheaper than next (compiler-marked this-free props passing through unbound) is a separate change.

Claude via Cursor

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