Skip to content

fix(solid): gate pending projection reads - #3225

Merged
ryansolid merged 2 commits into
solidjs:nextfrom
GabbeV:fix/server-pending-proxy-reads
Sep 2, 2026
Merged

fix(solid): gate pending projection reads#3225
ryansolid merged 2 commits into
solidjs:nextfrom
GabbeV:fix/server-pending-proxy-reads

Conversation

@GabbeV

@GabbeV GabbeV commented Sep 2, 2026

Copy link
Copy Markdown

Summary

  • suspend user-symbol reads, membership checks, key enumeration, and own-property descriptor reads while a server projection is pending
  • use the same NotReadyError/client-hole path as ordinary property reads
  • continue delegating directly to the real state after resolution, without a dummy target or mutation traps

This is a baseline inconsistency found while working on #3194, but the fix is independent of that API proposal. Previously store.name suspended while operations such as store[symbol], "name" in store, Object.keys(store), and Object.hasOwn(store, "name") could inspect the pending backing directly.

Descriptor reads are included because A25 requires the seed to remain hidden from every external consumer path. The trap returns the real target descriptor unchanged after readiness, preserving the target's proxy invariants.

This is expected to compose with the rejection-state fix in #3224 once that lands.

Test plan

  • pnpm exec vitest run test/server/ssr-async.spec.ts (157 tests)
  • pnpm types
  • pnpm test-types

@changeset-bot

changeset-bot Bot commented Sep 2, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: cd654ca

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

This PR includes changesets to release 11 packages
Name Type
solid-js Patch
@solidjs/element Patch
@solidjs/h Patch
@solidjs/html Patch
test-integration Patch
@solidjs/universal Patch
@solidjs/web Patch
@solidjs/babel-plugin Patch
@solidjs/compiler Patch
@solidjs/diagnostics Patch
@solidjs/signals 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

@ryansolid

Copy link
Copy Markdown
Member

This is directly required by A25: the seed must be hidden from every external consumer path, including symbols, membership, enumeration, and descriptor/Object.hasOwn reads. So the open question resolves toward adding a gated getOwnPropertyDescriptor trap too. #3224 also changes this proxy's state machine to preserve rejection errors; we'll merge that narrow fix first, then rebase this PR and add the descriptor/error-path coverage ourselves so the two changes compose cleanly.

@GabbeV
GabbeV force-pushed the fix/server-pending-proxy-reads branch from d68f53f to bbe6f71 Compare September 2, 2026 16:17
@GabbeV
GabbeV force-pushed the fix/server-pending-proxy-reads branch from bbe6f71 to 8d1ba82 Compare September 2, 2026 17:33
Ensure every gated read path preserves and rethrows the original projection error after settlement.

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

@ryansolid ryansolid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased cleanly over #3224 and added rejection coverage across ordinary, symbol, membership, enumeration, descriptor, and Object.hasOwn reads. The composed status gate now preserves the original rejection across the full pending membrane. Full solid suite (580 tests), focused SSR suite (160 tests), declarations/type tests, package build, and all size scenarios pass. Server-only; no public API or client bundle impact.

@ryansolid
ryansolid merged commit a88f12f into solidjs:next Sep 2, 2026
5 checks passed
@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 33662596132

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.564%

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: 1004
Covered Lines: 768
Line Coverage: 76.49%
Relevant Branches: 793
Covered Branches: 518
Branch Coverage: 65.32%
Branches in Coverage %: Yes
Coverage Strength: 14.6 hits per line

💛 - Coveralls

@codspeed-hq

codspeed-hq Bot commented Sep 2, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 136 untouched benchmarks
⏩ 132 skipped benchmarks1


Comparing GabbeV:fix/server-pending-proxy-reads (cd654ca) with next (a88f12f)2

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.

  2. No successful run was found on next (cd654ca) during the generation of this report, so a88f12f was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

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.

3 participants