diff --git a/.changeset/perf-shared-prop-proxy-traps.md b/.changeset/perf-shared-prop-proxy-traps.md new file mode 100644 index 000000000..9e685d2f7 --- /dev/null +++ b/.changeset/perf-shared-prop-proxy-traps.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +`merge()` and `omit()` proxies keep their per-instance state on the proxy target under symbol keys and share one handler each, instead of allocating a target with three closures (`get`/`has`/`keys`) per instance and forwarding every trap through them. Creating a props proxy is now a proxy plus a one- or two-slot object, and reads go straight from the trap to the sources. The state keys are never reported or answered through the proxy, and a string `defineProperty` on the proxy can no longer clobber its internals. Port of solidjs/solid#3391 to the 2.0 primitives. diff --git a/packages/signals/src/store/utils.ts b/packages/signals/src/store/utils.ts index 9aa0d7de1..2034020ac 100644 --- a/packages/signals/src/store/utils.ts +++ b/packages/signals/src/store/utils.ts @@ -7,34 +7,101 @@ function trueFn() { return true; } -const propTraps: ProxyHandler<{ - get: (k: string | number | symbol) => any; - has: (k: string | number | symbol) => boolean; - keys: () => (string | symbol)[]; -}> = { - get(_, property, receiver) { +// The merge() and omit() proxies keep their per-instance state on the proxy +// TARGET under symbol keys and share one handler each, so creating one costs +// a proxy plus a one- or two-slot object — no per-instance trap closures — +// and a read goes straight from the trap to the sources. The state keys are +// never reported by `ownKeys` and never answered by `get`/`has`, so they are +// invisible through the proxy. + +const $SOURCES = Symbol(__DEV__ ? "MERGE_SOURCE" : 0); +type MergeTarget = { [$SOURCES]: any[] }; + +function mergeGet(sources: any[], property: PropertyKey) { + for (let i = sources.length - 1; i >= 0; i--) { + const s = resolveSource(sources[i]); + if (property in s) return s[property]; + } +} + +const mergeTraps: ProxyHandler = { + get(target, property, receiver) { + if (property === $PROXY) return receiver; + const sources = target[$SOURCES]; + // The flat source list, for mergeSources() and for a nested merge. + if (property === $SOURCES) return sources; + return mergeGet(sources, property); + }, + has(target, property) { + if (property === $PROXY) return true; + const sources = target[$SOURCES]; + for (let i = sources.length - 1; i >= 0; i--) { + if (property in resolveSource(sources[i])) return true; + } + return false; + }, + set: trueFn, + deleteProperty: trueFn, + getOwnPropertyDescriptor(target, property) { + return { + configurable: true, + enumerable: true, + get: () => mergeGet(target[$SOURCES], property), + set: trueFn + }; + }, + ownKeys(target) { + const sources = target[$SOURCES]; + const keys = new Set(); + for (let i = 0; i < sources.length; i++) { + const sourceKeys = ownEnumerableKeys(resolveSource(sources[i])); + for (let j = 0; j < sourceKeys.length; j++) keys.add(sourceKeys[j]); + } + return [...keys]; + } +}; + +const $OMIT_PROPS = Symbol(__DEV__ ? "OMIT_PROPS" : 0); +const $OMIT_KEYS = Symbol(__DEV__ ? "OMIT_KEYS" : 0); +type OmitTarget = { [$OMIT_PROPS]: Record; [$OMIT_KEYS]: readonly PropertyKey[] }; + +function omitGet(target: OmitTarget, property: PropertyKey) { + // $SOURCES must not tunnel through the filter: merge() flattens whatever + // answers it, so forwarding would hand a re-merge the UNFILTERED sources + // of an underlying merge proxy and the omitted keys leak back in (#3014 — + // the SSR element-spread path re-merges static attributes with the rest + // object). Opaque here: merge composes omit proxies through their traps. + return property === $SOURCES || target[$OMIT_KEYS].includes(property) + ? undefined + : target[$OMIT_PROPS][property]; +} + +const omitTraps: ProxyHandler = { + get(target, property, receiver) { if (property === $PROXY) return receiver; - return _.get(property); + return omitGet(target, property); }, - has(_, property) { + has(target, property) { if (property === $PROXY) return true; - return _.has(property); + return ( + property !== $SOURCES && + !target[$OMIT_KEYS].includes(property) && + property in target[$OMIT_PROPS] + ); }, set: trueFn, deleteProperty: trueFn, - getOwnPropertyDescriptor(_, property) { + getOwnPropertyDescriptor(target, property) { return { configurable: true, enumerable: true, - get() { - return _.get(property); - }, - set: trueFn, - deleteProperty: trueFn + get: () => omitGet(target, property), + set: trueFn }; }, - ownKeys(_) { - return _.keys(); + ownKeys(target) { + const keys = target[$OMIT_KEYS]; + return ownEnumerableKeys(target[$OMIT_PROPS]).filter(k => !keys.includes(k)); } }; @@ -79,7 +146,6 @@ function resolveSource(s: any) { return !(s = typeof s === "function" ? s() : s) ? {} : s; } -const $SOURCES = Symbol(__DEV__ ? "MERGE_SOURCE" : 0); /** @internal The flattened sources behind a `merge()` PROXY, or undefined. * Only the proxy form: its writes are no-ops, so the sources are the whole * truth. merge()'s plain-object form also records `$SOURCES` (so nested @@ -125,32 +191,7 @@ export function merge(...sources: T): Merge { ); } if (SUPPORTS_PROXY && proxy) { - return new Proxy( - { - get(property: string | number | symbol) { - if (property === $SOURCES) return flattened; - for (let i = flattened.length - 1; i >= 0; i--) { - const s = resolveSource(flattened[i]); - if (property in s) return s[property]; - } - }, - has(property: string | number | symbol) { - for (let i = flattened.length - 1; i >= 0; i--) { - if (property in resolveSource(flattened[i])) return true; - } - return false; - }, - keys() { - const keys = new Set(); - for (let i = 0; i < flattened.length; i++) { - const sourceKeys = ownEnumerableKeys(resolveSource(flattened[i])); - for (let j = 0; j < sourceKeys.length; j++) keys.add(sourceKeys[j]); - } - return [...keys]; - } - }, - propTraps - ) as unknown as Merge; + return new Proxy({ [$SOURCES]: flattened }, mergeTraps) as unknown as Merge; } const defined: Record = Object.create(null); @@ -226,28 +267,10 @@ export function omit, K extends readonly (keyof T)[]> ...keys: K ): Omit { if (SUPPORTS_PROXY && $PROXY in props) { - return new Proxy( - { - get(property) { - // $SOURCES must not tunnel through the filter: merge() flattens - // whatever answers it, so forwarding would hand a re-merge the - // UNFILTERED sources of an underlying merge proxy and the omitted - // keys leak back in (#3014 — the SSR element-spread path re-merges - // static attributes with the rest object). Opaque here: merge - // composes omit proxies through their traps instead. - return property === $SOURCES || keys.includes(property as keyof T) - ? undefined - : props[property as any]; - }, - has(property) { - return property !== $SOURCES && !keys.includes(property as keyof T) && property in props; - }, - keys() { - return ownEnumerableKeys(props).filter(k => !keys.includes(k as keyof T)); - } - }, - propTraps - ) as unknown as Omit; + return new Proxy({ [$OMIT_PROPS]: props, [$OMIT_KEYS]: keys }, omitTraps) as unknown as Omit< + T, + K + >; } const result: Record = {}; const propNames = Object.getOwnPropertyNames(props); diff --git a/packages/signals/tests/store/utilities.test.ts b/packages/signals/tests/store/utilities.test.ts index 642ff694b..93187db20 100644 --- a/packages/signals/tests/store/utilities.test.ts +++ b/packages/signals/tests/store/utilities.test.ts @@ -171,6 +171,31 @@ describe("merge", () => { expect(merge(merge({ value: 1 }, { value: 2 }), { value: 3 }).value).toBe(3); expect(merge({ value: 1 }, merge({ value: 2 }, { value: 3 })).value).toBe(3); }); + it("keeps proxy state off the visible surface and safe from defines", () => { + const [store, setStore] = createStore({ id: 1, title: "Title" }); + const merged = merge({ size: "m" }, store); + const rest = omit(merged, "size"); + expect(Object.getOwnPropertySymbols(merged)).toEqual([]); + expect(Object.getOwnPropertySymbols(rest)).toEqual([]); + expect(Object.keys(merged).sort()).toEqual(["id", "size", "title"]); + expect(Object.keys(rest).sort()).toEqual(["id", "title"]); + // string defines land on the target and stay invisible; reads still go to the sources + Object.defineProperty(merged, "sources", { value: [], configurable: true }); + Object.defineProperty(rest, "props", { value: { id: 99 }, configurable: true }); + expect(merged.id).toBe(1); + expect(rest.id).toBe(1); + expect("sources" in merged).toBe(false); + expect("props" in rest).toBe(false); + expect(Object.keys(merged).sort()).toEqual(["id", "size", "title"]); + setStore(s => { + s.id = 2; + }); + flush(); + expect(merged.id).toBe(2); + expect(rest.id).toBe(2); + expect(Object.getOwnPropertyDescriptor(merged, "id")!.get!()).toBe(2); + expect(Object.getOwnPropertyDescriptor(rest, "size")!.get!()).toBeUndefined(); + }); it("does not clone nested objects", () => { const b = { value: 1 }; const props = merge({ a: 1 }, { b });