fix(store): never wrap platform objects — plain data and user classes only (#2952) - #2954
Merged
Merged
Conversation
… only (#2952) Native code brand-checks internal slots and throws when invoked with a store proxy as `this`: `store.map.size` (native accessor, proxy receiver) and draft `s.map.set(...)` (unbound method, draft `this`) both crashed with "called on incompatible receiver", while method READS half-worked (raw-bound) — an unmarked Map was wrapped but never honestly supported. Rather than teaching the trap per-callsite receiver conventions, fix wrappability: isWrappable now wraps plain data and user class instances only. Platform objects (Map, Set, Date, URL, DOM nodes, host objects) are excluded structurally by the tag check — user classes stringify as `[object Object]` while every native carries its own brand, subclasses included. They get the markRaw-children contract automatically: served raw, mutations land raw, and the property holding them still tracks (reassignment notifies). Perf: hot path (plain/null-proto) resolves on one getPrototypeOf — intrinsic, no property-lookup IC (the .constructor variant regressed ~50% from megamorphic loads). The custom-proto verdict is memoized per prototype, so each class pays the tag call once ever: class-read benchmarks at parity, combined store workload at parity. Size: +70 B brotlied on the createStore scenario, within budget. The explicit `instanceof Node` check stays, cold-branch only: real DOM nodes are excluded by tag, but shimmed DOMs implement nodes as plain user classes that pass it. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
🦋 Changeset detectedLatest commit: ef01b13 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
size-limit report 📦
|
Coverage Report for CI Build 30380098130Warning No base build found for commit Coverage: 75.417%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
Merging this PR will not alter performance
Comparing 
Footnotes
|
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
2 participants
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.
Fixes #2952.
Summary
this:store.map.sizecrashed on the read path (native accessor called with the proxy receiver) ands.map.set(...)crashed on the draft path (inherited method returned unbound, sothiswas the draft) — while method reads half-worked. An unmarked Map was wrapped but never honestly supported.isWrappablenow wraps plain data and user class instances only. Platform objects — Map, Set, Date, URL, DOM nodes, host objects — are excluded structurally by the tag check: user classes stringify as[object Object], every native/host object carries its own brand ([object Map],[object Headers], ...), and subclasses inherit the tag.markRaw-children contract automatically: served raw, mutations land on the raw object, and the property holding them still tracks, so reassignment notifies. User classes keep everything — prototype getters track through the proxy, draft methods keep the draftthisso their field writes stay reactive.instanceof Nodecheck stays on the cold branch only: real DOM nodes are excluded by tag, but shimmed DOMs (linkedom/happy-dom style) implement nodes as plain user classes that pass it.Performance
The naive version regressed (megamorphic
.constructorloads on the hot path; per-read tag calls for class values). As landed:Object.getPrototypeOf— intrinsic, no property-lookup IC;Local interleaved A/B (Node, min-of-5): plain-object stores within noise, class-instance reads at parity, combined workload at parity. Opening as a PR specifically to get CodSpeed's read on it.
Size: createStore scenario 12.8/12.85 kB brotlied (+70 B vs
next).Test plan
tests/store/native-collections.test.ts: Map/Set/Date reads (internal-slot accessors, iteration), draft mutations, raw identity, subclass tags, reassignment tracking, user-class getter/method reactivity on the same store, null-proto objects, and the discriminator case from the issue.Made with Cursor