Skip to content

OR flattenArray's needsUnwrap with nested results instead of overwriting (#3133) - #3149

Closed
ryansolid wants to merge 1 commit into
nextfrom
fix/flatten-needsunwrap-or
Closed

OR flattenArray's needsUnwrap with nested results instead of overwriting (#3133)#3149
ryansolid wants to merge 1 commit into
nextfrom
fix/flatten-needsunwrap-or

Conversation

@ryansolid

Copy link
Copy Markdown
Member

Summary

Fixes #3133; supersedes #3142.

  • The one-line fix exactly as @antoinevanwel proposed in the report: flattenArray OR-s its needsUnwrap flag with a nested call's result instead of overwriting it, so an accessor followed at the same level by a function-free fragment still gets the resolving wrapper.
  • New finding while verifying scope: the report assumed @solidjs/web was protected by insertExpression's function branch — that branch is 1.x dom-expressions and does not exist in 2.0. The DOM renderer crashes on the same shape (Failed to execute 'insertBefore' on 'Node': parameter 1 is not of type 'Node'), verified pre-fix in jsdom. So this was a crash across every renderer, not a universal-only defect.
  • Pinned at both layers: signals-level flatten tests (wrapper kept for accessor-then-fragment and fragment-nested-accessor orders, plain array when nothing unwraps — both wrapper pins fail pre-fix) and a web/jsdom render test (fails pre-fix with the insertBefore TypeError).

Test plan

  • Signals flatten pins fail pre-fix, pass post-fix
  • Web render pin fails pre-fix (insertBefore TypeError), passes post-fix
  • Full signals (1434) + web (672) suites green

Made with Cursor

…ing (#3133)

Under doNotUnwrap, an accessor child followed at the same level by a
function-free fragment reset the flag, so flatten returned a plain array with
the raw accessor inside instead of the resolving wrapper. Every renderer
crashes on the raw function: universal hosts receive it in insertNode (as
reported), and the DOM renderer throws insertBefore 'parameter 1 is not of
type Node' — the protective function branch remembered from 1.x
dom-expressions does not exist in 2.0, so this was not universal-specific.
Fix as proposed in the report; pinned at both the signals and web layers.

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

changeset-bot Bot commented Aug 31, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2ecedf4

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
solid-js Patch
@solidjs/babel-plugin Patch
@solidjs/compiler Patch
@solidjs/html Patch
@solidjs/h Patch
@solidjs/universal Patch
@solidjs/element Patch
@solidjs/diagnostics 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

Copy link
Copy Markdown

Coverage Report for CI Build 33371165631

Coverage remained the same at 71.652%

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: 1003
Covered Lines: 767
Line Coverage: 76.47%
Relevant Branches: 789
Covered Branches: 517
Branch Coverage: 65.53%
Branches in Coverage %: Yes
Coverage Strength: 14.53 hits per line

💛 - Coveralls

@codspeed-hq

codspeed-hq Bot commented Aug 31, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 28.59%

⚠️ 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

❌ 3 regressed benchmarks
✅ 133 untouched benchmarks
⏩ 132 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
merge 225.1 µs 326.7 µs -31.11%
merge 264.7 µs 364.2 µs -27.33%
merge 265.2 µs 364.6 µs -27.27%

Tip

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


Comparing fix/flatten-needsunwrap-or (2ecedf4) with next (28a1eaf)

Open in CodSpeed

Footnotes

  1. 132 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 Author

Landed directly on next as a cherry-pick (same treatment as #3148 — clearing the board for rc.5). Signals + web suites green; the size budget this was 9 B over inherits the 26.15 KB limit that landed with #3148's scheduler bytes.

@ryansolid ryansolid closed this Aug 31, 2026
@ryansolid
ryansolid deleted the fix/flatten-needsunwrap-or branch August 31, 2026 09:43
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