Skip to content

merge() re-flattens through a stale $SOURCES and discards the object's own properties #3384

Description

@ryansolid

Version: 2.0.0-rc.8 (@solidjs/signals / solid-js)

Repro

import { merge, omit } from "solid-js";

// A merged props object (plain-object form: no source is a $PROXY).
const props = merge({ href: "/x", title: "t" }, { $active: true, children: "hi" });

// Copy it the way a library might (Reflect.ownKeys → includes the $SOURCES symbol),
// dropping `$active` and adding an own `class` getter.
const out = {};
for (const key of Reflect.ownKeys(props)) {
  if (key === "$active") continue;
  Object.defineProperty(out, key, Reflect.getOwnPropertyDescriptor(props, key));
}
Object.defineProperty(out, "class", { get: () => "yak-abc", enumerable: true, configurable: true });

const final = merge(out, { rel: "noopener" });
final.class;    // undefined   (expected "yak-abc")
final.$active;  // true        (expected undefined — it was removed)
Full repro (the yak `copyProps` logic verbatim, both `merge` and `omit` paths)
// Repro: yak's copyProps copies merge()'s $SOURCES symbol, so a downstream
// merge() flattens to the ORIGINAL sources and bypasses yak's filtering.
import { merge, omit } from "solid-js";

const skip = k => typeof k === "string" && (k.charCodeAt(0) === 36 || k === "class" || k === "theme");

// yak PR copyProps (verbatim logic)
function copyProps(props) {
  const out = {};
  for (const key of Reflect.ownKeys(props)) {
    if (skip(key)) continue;
    const d = Reflect.getOwnPropertyDescriptor(props, key);
    if ("value" in d && d.enumerable) out[key] = d.value;
    else Object.defineProperty(out, key, d);
  }
  Object.defineProperty(out, "class", { get: () => "yak-abc", enumerable: true, configurable: true });
  return out;
}

// Call site: <StyledLink {...linkProps} $active children="hi" />  → compiled createComponent(StyledLink, merge(linkProps, {...}))
const linkProps = { href: "/x", title: "t" };
const props = merge(linkProps, { $active: true, children: "hi" });
console.log("props is plain object with $SOURCES:", Object.getOwnPropertySymbols(props).length > 0);

const forwarded = copyProps(props);
console.log("forwarded keys:", Reflect.ownKeys(forwarded).map(String));

// Target component body: <a {...props} rel="noopener" /> → spread(el, merge(props, { rel }))
const final = merge(forwarded, { rel: "noopener" });
console.log("final.class   =", final.class, "   (expected 'yak-abc')");
console.log("final.$active =", final.$active, "   (expected undefined)");
console.log("final keys    =", Object.keys(final));

// Same thing through omit() in the target (rest-props pattern), then merge:
const rest = omit(forwarded, "title");
const final2 = merge(rest, { rel: "noopener" });
console.log("via omit: class =", final2.class, " $active =", final2.$active);

Cause

packages/signals/src/store/utils.ts merge() (~112–193, plain-object path): when a source carries $SOURCES, merge flattens through it to the original sources instead of treating the object as a source in its own right. Any own property added or removed after the fact is ignored. omit() had the same shape of problem and was fixed in #3014; merge() still trusts $SOURCES unconditionally.

Why it matters

$SOURCES is an enumerable-by-Reflect.ownKeys symbol on a plain object that looks like ordinary props. Anything that copies props generically (Reflect.ownKeys, Object.getOwnPropertySymbols, structured clones of descriptors) inherits it, and the next merge downstream silently resurrects removed keys and drops added ones. @yak/solid's PR #644 does exactly this (copyProps), so $-prefixed style props leak onto DOM elements and the computed class is lost whenever the target component spreads {...props} — we confirmed it against rc.8.

Options

  1. Make $SOURCES non-copyable: define it non-enumerable and have merge verify the object's own keys still match the flattened sources before trusting it (cheap: compare key count).
  2. Treat an object with own data/accessor properties that aren't in its $SOURCES as opaque (no flattening) — the SSR spread ignores omit()'s hidden keys when props are a merge proxy — omitted props leak into the HTML and the leaked handler is re-bound on hydration #3014 approach.
  3. Drop $SOURCES flattening entirely and accept one extra level in nested merges.

(1) or (2) keeps the current fast path for the common compiler-emitted case.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions