perf(signals): shared traps for merge()/omit() proxies, state on the target

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

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<MergeTarget> = {
+  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<string | symbol>();
+    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<PropertyKey, any>; [$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<OmitTarget> = {
+  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<T extends unknown[]>(...sources: T): Merge<T> {
       );
   }
   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<string | symbol>();
-          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<T>;
+    return new Proxy({ [$SOURCES]: flattened }, mergeTraps) as unknown as Merge<T>;
   }
 
   const defined: Record<string, PropertyDescriptor> = Object.create(null);
@@ -226,28 +267,10 @@ export function omit<T extends Record<any, any>, K extends readonly (keyof T)[]>
   ...keys: K
 ): Omit<T, K> {
   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<T, K>;
+    return new Proxy({ [$OMIT_PROPS]: props, [$OMIT_KEYS]: keys }, omitTraps) as unknown as Omit<
+      T,
+      K
+    >;
   }
   const result: Record<string, any> = {};
   const propNames = Object.getOwnPropertyNames(props);

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 });