diff --git a/CHANGELOG.md b/CHANGELOG.md index 05840bd48..126bd1efc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,11 @@ xx.xx.xxxx * **Feature** - Added `{#fn expression}` directive to pass values as-is without auto-invoking functions — mirrors `{#html}` pattern, useful for passing callbacks through property bindings ### Reactivity +* **BREAKING** - Signals now default to `safety: 'freeze'` (deep-freeze on set) instead of clone-on-read. Mutations of a signal value throw `TypeError` at the call site instead of being silently swallowed. Reads return the frozen reference — no more per-read clone. Three presets: `'freeze'` (default), `'reference'` (no protection; framework/perf opt-out), `'none'` (no protection, no dedupe; event-stream semantics). +* **BREAKING** - Removed the `allowClone` constructor option. Use `safety: 'reference'` for the equivalent "no protection" behavior. Signals that previously relied on clone-on-read for mutation isolation need to switch to `safety: 'reference'` or rewrite callers to avoid mutating peeked references. +* **Feature** - Added `signal.clone()` — tracked read that returns a deep copy. Use when handing signal data to libraries that mutate in place. +* **Feature** - Added static accessor properties for configuring defaults — `Signal.safety`, `Signal.tracing`, `Signal.stackCapture`, `Signal.equalityFunction`, `Signal.cloneFunction`. Assignment validates (function accessors throw `TypeError` on non-function). Bulk setup via `Signal.configure({...})`; inspect current state via `Signal.defaults`. +* **Feature** - In-place array helpers (`push`, `unshift`, `splice`, `setIndex`, `removeIndex`, `setArrayProperty`, `setProperty`) branch on safety — `'reference'` / `'none'` retain O(1) in-place mutation; `'freeze'` builds new arrays/objects immutably. * **Feature** - Added `depend()` to register a signal as a dependency without reading the value * **Feature** - Added `notify()` to force-trigger subscribers bypassing the equality check * **Feature** - Added `hasDependents()` to check if any reactions are subscribed to a signal @@ -28,6 +33,10 @@ xx.xx.xxxx ### Reactivity * **Bug** - Fixed `instanceof` brand check on `Signal` to use prototype getter instead of class field — ensures cross-realm and prototype-created instances pass `instanceof` reliably. +### Utils +* **BREAKING** - `noop` now returns `undefined` (truly no-op). Previously it returned its first argument (identity). Use the new `identity` export if you need the old behavior. +* **Feature** - Added `identity` export — returns its first argument unchanged. + ### Templates * **Bug** - Fixed `instanceof` brand check on `Template` to use prototype getter instead of class field — same fix as Signal and Query. diff --git a/ai/plans/ROADMAP.md b/ai/plans/ROADMAP.md index 14612d88c..b16c8de08 100644 --- a/ai/plans/ROADMAP.md +++ b/ai/plans/ROADMAP.md @@ -172,6 +172,7 @@ Post-pipeline. Write docs when there's something to document. Build wrappers whe | P4 | Homepage Tour Ribbon | 16-24h (2-3d) | pair | 3 PlaygroundExamples for templates/specs/components. | | P5 | CSS Token Extraction | 16-24h (2-3d) | pair | `getThemingCSS` util + MCP tool. Blocked on token finalization. | | P6 | MCP Improvements (remaining 3 tools) | 8-16h (1-2d) | agent | `get_theming_css`, `get_global_tokens`, `get_token_usage`. Blocked on token extraction. | +| P7 | [Utils Modernization](utils-modernization.md) | 4-8h + codemod | pair | Remove `any`/`onlyKeys` aliases, fix `first`/`last` return-type polymorphism, add `pipe`/`attempt`/`tap`. `initial` scope — 5 quick design calls. | --- diff --git a/ai/plans/utils-modernization.md b/ai/plans/utils-modernization.md new file mode 100644 index 000000000..2a5a93f7c --- /dev/null +++ b/ai/plans/utils-modernization.md @@ -0,0 +1,83 @@ +# Utils Modernization + +## Goal + +`@semantic-ui/utils` has accumulated a mix of principled next-gen design and lodash-parity leftovers. The library's instinct is right — when two functions exist, they should capture meaningfully distinct behaviors, not arg-shape variants. A few leftovers violate that, and a few modern patterns are missing. + +This plan captures the decisions needed to reconcile the library with that principle and stage the breaking changes behind a single release window. + +**Naming principle**: two exports earn their place when they express **distinct intent at the call site**, even if the implementations overlap. Two names for the same intent (`any = some`) is a wart; two names for distinct intents that happen to share a body (`wrapFunction` as defensive normalization vs `constant` as explicit factory) is a feature — the name is for the reader. + +## Design / Implementation + +### Removals (breaking) + +| Export | Issue | Proposed action | +|--------|-------|-----------------| +| `any` | Exact alias of `some` (`arrays.js:237`) — two names, one function | Delete. Document `some` as canonical (matches `Array.prototype.some`). | +| `onlyKeys` | Same operation as `pick` but takes array instead of spread args | Pick one shape. Most callers pass an array today (from config/filters/`Object.keys`) — recommend standardizing on `pick(obj, keys)` taking an array, delete `onlyKeys`. | + +### Signature changes (breaking) + +| Export | Issue | Proposed action | +|--------|-------|-----------------| +| `first` / `last` | Return-type polymorphism: `first(arr)` returns element, `first(arr, 2)` returns array | Split: `first(arr)` always returns the element. Add `firstN(arr, n)` / `lastN(arr, n)` that always return arrays. | + +### Additions + +| Export | Rationale | +|--------|-----------| +| `returnsSelf`, `returnsNull`, `returnsTrue`, `returnsFalse`, `returnsUndefined` | Named, shared, reference-stable functions for the common constant returns. Ship the common cases as direct exports so every consumer dedupes to the same closure — enables reference-equality checks like `template.js`'s `this.onThemeChangedCallback !== noop` pattern to generalize across the codebase. | +| `constant = wrapFunction` | Alias for the explicit-factory intent. Same body as `wrapFunction`, different semantic intent at the call site: `wrapFunction(x)` reads as defensive normalization ("value-or-function, normalize"); `constant(x)` reads as explicit factory ("function that returns this"). Covers the programmatic-value case (`constant(config.default)`) where a direct named export can't. | +| `identity` → rename to `returnsSelf` | `identity` is brand-new in this release (from the noop/identity split). Rename to `returnsSelf` for consistency with the prefix family and to avoid FP jargon. Ship both renames together in this release window rather than churning twice. | +| `pipe(...fns)(x)` | Functional composition is idiomatic in modern TS; lodash's `flow` is clunky. Small primitive, pairs well with existing polymorphic iterators. | +| `attempt(fn, fallback)` or `safely(fn)` | Several sites in the codebase use `try { ... } catch { /* noop */ }` (see `browser.js:113,152,178,191`). A named helper makes intent greppable and documents the pattern. | +| `tap(fn)` | Passes value through while running a side effect. Small, useful in pipes and reactive chains. | + +### Decisions needed + +- **Do `any` and `onlyKeys` get deprecation shims in this release, or hard-deleted?** Hard-delete is cleaner for a 1.0-era library; deprecation adds complexity for a short window. Recommend hard-delete + loud CHANGELOG entry + migration note. +- **`first`/`last` strategy:** (a) always-scalar + new `firstN`/`lastN`, or (b) always-array (breaking for all current 1-arg callers)? Recommend (a) — smaller blast radius, clearer semantics at call site. +- **`pick` arg shape final answer:** spread (`pick(obj, 'a', 'b')`) or array (`pick(obj, ['a', 'b'])`)? Recommend array — composes with `Object.keys`, filter results, config. Spread is 2014 ergonomics for hand-literal keys that rarely appear in modern code. +- **`pipe` return style:** eager (`pipe(fns, x)`) or curried (`pipe(...fns)(x)`)? Curried composes better; eager is simpler. +- **`attempt` vs `safely` naming:** `attempt(fn, fallback)` reads naturally ("attempt this, else fallback"). `safely(fn)` suggests "swallow errors" without the fallback value — less useful. +- **Shape resolved.** Ship shared direct exports for the common constant returns (`returnsSelf`, `returnsNull`, `returnsTrue`, `returnsFalse`, `returnsUndefined`) — reference-stable across consumers, enables dedup and ref-equality checks. `constant` is an alias of `wrapFunction` covering the programmatic-value case. `identity` renames to `returnsSelf` in the same release window. + - Rejected names along the way: `always` (asserts continued truth when actual use is ephemeral-default), bare `returns(value)` / `constant(value)` as the *only* surface (works, but loses the dedup win from sharing primitive closures across the codebase). + +### Out of scope + +- Changes to `extend` / `deepExtend` / `assignInPlace` — the three merge variants already capture distinct contracts (shallow / recursive / sync-with-deletion). Leave as-is. +- Rewriting `proxyObject`, `weightedObjectSearch`, `hashCode` — these are niche, principled, and earn their export. + +### Completed as one-line wins (not part of this plan) + +- `proxyObject` default swapped from `noop` to `() => ({})`. *Revisit:* `() => ({})` allocates a fresh closure on each defaulted call — once `always` lands, change to `always({})` for a shared reference. +- `hasProperty` reduced to thin re-export of `Object.hasOwn` (ES2022). +- `utility-functions.md` skill updated for `noop` / `identity` split. + +### Adoption once `always` lands + +Audit the codebase for `() => ` patterns that are candidates to migrate: +- `NO_EQUALITY = () => false` in `reactivity/src/signal.js` → `always(false)` +- `filter: () => true` defaults (e.g. `docs/src/examples/templates/expressions-fn/filter-list.js:7`) +- `proxyObject` default (above) +- Any `defaultSettings` entries that use bare `() => x` closures + +## Open Questions + +See "Decisions needed" above. Five design calls, all quick — should fit in one pair session. + +Once decisions are made: +- Codemod for `any` → `some`, `onlyKeys` → `pick` across `packages/`, `src/`, `docs/`, `tools/`. +- Audit `first(arr, N)` / `last(arr, N)` call sites — all need rewrite to `firstN` / `lastN`. +- CHANGELOG entries under **BREAKING** for each removal/signature change. +- Type-file updates. +- Skill doc (`ai/skills/authoring/utility-functions.md`) updates. + +## Dependencies + +None. Can land anytime — breaking changes batch naturally with other utils changes in a release window. + +## Status + +`initial` — design decisions pending. Created 2026-04-16 off the utils review following the signal-safety PR. diff --git a/ai/skills/authoring/component-state.md b/ai/skills/authoring/component-state.md index 9e8fed008..40bdf7a1f 100755 --- a/ai/skills/authoring/component-state.md +++ b/ai/skills/authoring/component-state.md @@ -314,12 +314,12 @@ const defaultState = { counter: 0, isOpen: false }; // signal() in createComponent — needed when you want signal options or // to create signals dynamically const createComponent = ({ signal }) => ({ - element: signal(null, { allowClone: false }), // custom options - count: signal(0), // explicit signal + element: signal(null, { safety: 'reference' }), // custom options + count: signal(0), // explicit signal }); ``` -Use `defaultState` by default. Use `signal()` when you need `allowClone: false`, custom equality, or signals created conditionally. +Use `defaultState` by default. Use `signal()` when you need a non-default safety preset (e.g. `safety: 'reference'` for third-party data), custom equality, or signals created conditionally. --- diff --git a/ai/skills/authoring/reactive-state.md b/ai/skills/authoring/reactive-state.md index 59d00a339..3e2ba4c9d 100755 --- a/ai/skills/authoring/reactive-state.md +++ b/ai/skills/authoring/reactive-state.md @@ -48,17 +48,17 @@ new Signal(initialValue, options) - `initialValue`: Any - The initial value for the signal - `options`: Object (optional) - `context`: Object - Debugging context metadata + - `safety`: `'freeze'` | `'reference'` | `'none'` - Value protection preset (default: `'freeze'`) - `equalityFunction`: Function - Custom equality comparison (default: deep equality) - - `allowClone`: Boolean - Whether to clone values (default: true) - - `cloneFunction`: Function - Custom cloning function (default: deep clone) + - `cloneFunction`: Function - Custom cloning function used by `signal.clone()` (default: deep clone) **Examples**: ```javascript -// Basic signal +// Basic signal (default safety: 'freeze') const count = new Signal(0); -// Signal with no cloning (for performance or object identity) -const element = new Signal(domElement, { allowClone: false }); +// Signal for DOM or third-party objects — don't freeze borrowed references +const element = new Signal(domElement, { safety: 'reference' }); // Signal with custom equality const user = new Signal(userData, { @@ -188,19 +188,20 @@ const user = new Signal(userData, { }); ``` -#### Cloning Control +#### Safety Presets ```javascript -// Disable cloning for performance or object identity preservation -const expensiveObject = new Signal(largeDataStructure, { - allowClone: false // No cloning, use object as-is -}); +// Default — deep-freeze on set; in-place mutation throws at the call site +const config = new Signal({ theme: 'dark' }); + +// Opt out for signals holding borrowed data (libraries, APIs, callbacks) +const pagefindResults = new Signal([], { safety: 'reference' }); -// Custom cloning function +// Event-stream semantics — every set notifies, even if value is equal +const pulse = new Signal(null, { safety: 'none' }); + +// Custom clone function used by signal.clone() const customSignal = new Signal(data, { - cloneFunction: (value) => { - // Custom cloning logic - return JSON.parse(JSON.stringify(value)); - } + cloneFunction: value => JSON.parse(JSON.stringify(value)), }); ``` diff --git a/ai/skills/authoring/utility-functions.md b/ai/skills/authoring/utility-functions.md index 1d2ce6cff..9b107ec3e 100755 --- a/ai/skills/authoring/utility-functions.md +++ b/ai/skills/authoring/utility-functions.md @@ -367,11 +367,17 @@ truncate('こんにちは世界です', 8, { locale: 'ja' }); // 'こ ### Core ```javascript -import { noop, wrapFunction } from '@semantic-ui/utils'; +import { noop, identity, wrapFunction } from '@semantic-ui/utils'; -// Identity function — returns its argument -noop(42); // 42 -noop('hello'); // 'hello' +// noop — swallows arguments, returns undefined. Use as a reusable empty +// callback to avoid allocating fresh () => {} closures. +noop(); // undefined +noop(42, 'ignored'); // undefined + +// identity — returns its first argument unchanged. Use as a pass-through +// default for transforms (e.g. `transform = mapFn ?? identity`). +identity(42); // 42 +identity('hello'); // 'hello' // Wraps non-functions into a function that returns the value const fn = wrapFunction('default'); // () => 'default' @@ -827,7 +833,8 @@ const pattern = new RegExp(escapeRegExp('price ($5.00)'), 'i'); ### Functions (functions.js) | Function | Signature | Returns | |----------|-----------|---------| -| `noop` | `(v)` | `v` (identity function) | +| `noop` | `()` | `undefined` — swallows args, reusable empty callback | +| `identity` | `(v)` | `v` — returns first arg unchanged | | `wrapFunction` | `(x)` | `x` if function, else `() => x` | | `memoize` | `(fn, hashFn?)` | Memoized function | | `wait` | `(ms, opts?)` | Promise | diff --git a/ai/skills/contributing/agent-lessons.md b/ai/skills/contributing/agent-lessons.md index 2be6e61c1..d1ff56a59 100644 --- a/ai/skills/contributing/agent-lessons.md +++ b/ai/skills/contributing/agent-lessons.md @@ -68,14 +68,111 @@ This applies to naming functions (elicit usage patterns before picking names), c --- -## Delegate to Fresh Perspectives +## Quiet Code Over Ornamented Code -When stuck in a solution direction, delegating to a fresh agent (or pair of agents) reading the problem from scratch produces better results than pushing harder yourself. Rules that worked: +Training data pushes toward a particular visual dialect: `SCREAMING_CAPS` for module-level values, `_underscore` prefixes for "private" properties, dispatcher functions that route to shared refs via if-ladders, options objects where direct literals would do. Each is borrowed from a different language or era. None earn their weight in modern JS — they add visual ceremony that signals "I'm being rigorous" without conveying more information. -- Describe the problem and symptoms, not the diagnosis -- Give all relevant file paths — don't make them search -- Let failing tests with correct expectations count as valid findings -- Independent convergence on the same fix is the highest-confidence signal +`MAX_RETRIES = 3` doesn't tell the reader more than `maxRetries = 3`; `const` already declares immutability. `_privateCache` doesn't tell the reader more than `privateCache`; JS has no language-level convention here. A `constant(value)` dispatcher that routes to `ALWAYS_TRUE`/`ALWAYS_FALSE` via an if-ladder doesn't tell the reader more than seven flat exports — `returnsTrue`, `returnsFalse`, `returnsNull`, `returnsSelf`, etc. — sitting next to each other. In each case the quiet form is strictly more legible: intent lives in the name, at the value level, not in a scaffolding layer above it. + +Specific tells to notice in your own output: + +- `SCREAMING_CAPS` module-level constants +- `_underscore`-prefixed "private" names (not SUI convention — see `feedback_no_underscore_vars`) +- Dispatcher functions that route to shared refs via if/switch — prefer N direct flat exports +- Options objects for configuration that never varies +- "Extensibility for the future" when the future isn't real +- Factories that construct what could be literals + +The test: if this code had only one user and no hypothetical future callers, would it still have this shape? Usually the ornamented version collapses to something simpler. In the framework author's words, ornament "obscures the elegance and semantic intent." + +**Apply this as a review lens, not just a write-time check.** The quiet form is rarely what lands on the first pass — it's what emerges when you re-read the diff with "can this be quieter?" as the question. Example: a `Signal.configure` that ran a 5-line if-ladder to forward keys into setters survived the write pass; the review pass collapsed it to `Object.assign(Signal, config)` because bracket assignment already routes through setters. Before finalizing any changeset, re-read with the lens — the Object.assign instinct has to be yours by default, not the reviewer's. + +### Comments: the OSS bar + +The same discipline applies to comments, and this is a repeated failure point for AI defaults — both directions. On the first pass, agents tend to narrate (`// loop through items`, `// set the value`, `// Factory for signals computed from other signals`). On the review pass, agents tend to over-prune and strip load-bearing WHY notes alongside the narration. Both miss the target. + +**The bar**: a comment earns its place only if it documents something **non-obvious to someone who doesn't know the codebase** — a weird trick, a hidden constraint, a problem the code is defending against, a performance choice backed by numbers. + +Compare: + +❌ **Narration / internal rationale** (remove): +```js +// Module-local only because it's used as a computed class member key +// (`[IS_SIGNAL]`), which is evaluated before static fields initialize. +const IS_SIGNAL = Symbol.for('semantic-ui/Signal'); +``` +This explains the internal engineering decision (where it lives) to readers who weren't asking. Dead weight. + +✅ **Problem being solved** (keep): +```js +// solves 'instanceof Signal' checks if across realms or package duplication +const IS_SIGNAL = Symbol.for('semantic-ui/Signal'); +``` +A reader sees `Symbol.for` + `[Symbol.hasInstance]` and wonders *why not just `instanceof`*. The comment answers that — points at the observable failure this technique defends against. That's the bar. + +Other load-bearing categories: +- **Performance numbers**: `// Error.captureStackTrace is 10-100× a context spread; gated on stack mode` +- **Behavioral variance by state**: the block above `Signal.prototype.mutate` explaining freeze vs. reference/none semantics +- **Weird-trick explanations**: `// WeakRef lets the derived reaction self-stop when the source is GC'd` +- **Config / enum value docs**: `// 'off' — zero cost; 'context' — attach bags; 'stack' — captureStackTrace per notify` inline on a config object. Readers using the config need the semantics somewhere, and the config site is where they look. + +What the above examples share: a future reader sees the code, asks a specific question, and the comment answers that question in one line. Never restate what the code does. Never announce internal plans ("replace with X when Y ships"). Never document API contracts that belong in JSDoc or types. + +**Section dividers are a separate case.** "No section labels" is the right instinct for single-declaration labels like `// DX pass throughs` above one static assignment. It's the wrong instinct for multi-method conceptual clusters in a large file — the SUI codebase uses a canonical three-level comment hierarchy for large CSS files, config files, and organized JS files (see the `code-formatting` skill). Level-2 dividers (`/*--- Core ---*/`, `/*--- Mutation Helpers ---*/`, `/*--- Configuration ---*/`) are the correct tool for grouping ~10 related methods as navigation aids. Test: does the divider label a conceptual cluster a reader wants to scan past or jump to? Follow the three-level hierarchy. If it labels one thing the name already conveys, remove. + +--- + +## You Are an Orchestrator, Not an Investigator + +The default failure mode for agents on hard diagnostic tasks is treating them as solo work — reading, hypothesizing, implementing, measuring, all in the main conversation. This is roughly an order of magnitude slower than it needs to be, and it accumulates anchoring bias as your own theories color each subsequent step. + +Reframe the role: **you deploy investigators; you don't investigate**. Diagnostic work is inherently parallelizable, and fresh subagents produce independent reads of evidence without carrying your prior theories. + +### Parallelize by default + +Any moment you catch yourself saying "let me go read X and figure out Y," check whether the read-and-figure-out could be done by a fresh agent with a self-contained brief. Usually it can. Common patterns where parallel agents dominate solo investigation: + +- **Branching hypotheses** — "Is the regression in path A or path B?" → one agent per path. +- **Multi-angle investigation** — profile + diff + aggregate artifacts + call-chain audit + grep-for-callsites are five distinct angles on the same symptom, all suitable for parallel fresh agents. +- **Unfamiliar code exploration** — "How does reconcile phase 3 work?" → dispatch an Explore agent with a narrow question, move on. +- **Independent validation of a fix** — after a change, dispatch an agent to verify the fix resolves the original symptoms without regressing adjacent behavior. + +Five parallel agents at ~10 minutes each = 10 minutes of wall clock for 50 agent-minutes of work. The cost-benefit versus solo investigation is lopsided almost always. + +### Synthesis, not aggregation + +Your job when reports come back isn't to concatenate them. It's to: + +- **Identify convergence** — two independent agents reaching the same conclusion via different paths is the highest-confidence signal available. Stop hedging, move on. +- **Identify divergence** — disagreement between agents flags ambiguous evidence. Next move: targeted third agent to resolve, or in-context examination of the specific disagreement point. +- **Reject weak findings** — "X might be the issue but I'm not sure" is a hypothesis, not evidence. Treat it as a pointer to the next investigation, not a conclusion. + +### Briefing discipline + +Fresh agents start with zero context. Every brief must be self-contained: + +- **State the problem and symptoms** — not your diagnosis. +- **List exact file paths** — line numbers if you have them. Don't make the agent search. +- **Choose prescribed-hypothesis vs open-ended** with intent. Prescribing produces confirmation bias; open-ended produces independent judgment. Both are useful in different situations. +- **Specify the deliverable** — report, profile output, code change, measurement. Include format ("under 300 words", "return the top 10 hot frames by tick count"). +- **Tell them whether to write code or only investigate.** Unclear here is a common churn source. + +See the `fresh-take` skill for the deeper bias-isolation technique when delegating a single careful evaluation (distinct from the parallel-investigation pattern here). + +### When NOT to parallelize + +In-context work is better when: + +- The task is mechanical and fast (reading one file, running one command). +- The task requires accumulated conversation context (an in-progress refactor, iterative review). +- The task requires user judgment mid-flight (design decisions, scope trade-offs). +- The deliverable is a code change the user needs to diff-review before commit. + +Rule of thumb: if the work is **investigative** (reading, profiling, analyzing, summarizing), default to subagent. If the work is **productive or interactive** (writing production code, reviewing with user, navigating a tricky refactor), default to in-context. + +### The "let me think about this" tell + +More than ~60 seconds reasoning about a diagnostic problem without executing anything is almost always the moment to dispatch an agent instead. Solo reasoning on ambiguous diagnostic questions produces anchoring, not answers. Parallel fresh agents produce evidence. --- @@ -96,8 +193,10 @@ Ask "what's the actual cost?" before investing energy in reducing it. The user w | Deferring work that should be finished now | Read the room — the user may see the full arc | | Applying training-data assumptions | Verify against actual codebase behavior | | Optimizing for hypothetical costs | Ask about the real cost model first | -| Pushing harder when stuck | Delegate to a fresh perspective | +| Investigating solo | Orchestrate — dispatch parallel fresh agents, synthesize findings | +| Reasoning >60s without executing | Dispatch an agent instead of thinking harder | | Adding semantic abstraction early | Start literal, promote with evidence | +| Decorating code with training-data visual conventions | Quiet form wins — flat direct names over SCREAMING_CAPS, underscores, dispatcher layers | --- @@ -107,4 +206,5 @@ Ask "what's the actual cost?" before investing energy in reducing it. The user w |-------|---------|-------------| | **Mental Model** | `mental-model` | Understanding the framework's core concepts | | **Build System** | `build-system` | Working with the build pipeline | +| **Fresh Take** | `fresh-take` | Careful bias-isolated delegation for a single deep evaluation (complements the parallel-orchestration pattern above) | | **Agent Guestbook** | `agent-guestbook` | Reading the full stories behind these lessons | diff --git a/ai/skills/contributing/internals.md b/ai/skills/contributing/internals.md index 13bb970ec..aa2eac624 100755 --- a/ai/skills/contributing/internals.md +++ b/ai/skills/contributing/internals.md @@ -233,7 +233,7 @@ Signal.set(newValue) **Dependency tracking**: `Signal.get()` calls `Dependency.depend()`. If `Scheduler.current` is set (meaning we're inside a Reaction's `run()`), the Reaction is added to the Dependency's subscriber set, and the Dependency is added to the Reaction's dependency set. This bidirectional link enables cleanup. -**Clone by default**: `Signal.set()` and `Signal.get()` clone values for objects and arrays (via `maybeClone`). This prevents mutation-without-notification bugs. Class instances are not cloned (detected via `isClassInstance`). Opt out with `{ allowClone: false }`. +**Freeze by default**: `Signal.set()` deep-freezes plain objects and arrays (via `deepFreeze`). In-place mutation on a `.get()` result throws `TypeError` at the call site, catching the mutate-without-notify class of bugs. Date/Map/Set/RegExp/class-instances are skipped (they aren't plain objects and freezing them would break their semantics). Opt out per-signal with `{ safety: 'reference' }` for borrowed data, or `{ safety: 'none' }` for event-stream semantics. **Scheduler batching**: Multiple `Signal.set()` calls in the same synchronous block schedule one microtask flush. All pending Reactions run in the next microtask, ensuring consistent state. diff --git a/ai/skills/workflows/contributing/improve-performance.md b/ai/skills/workflows/contributing/improve-performance.md index 962491d25..343a4a882 100644 --- a/ai/skills/workflows/contributing/improve-performance.md +++ b/ai/skills/workflows/contributing/improve-performance.md @@ -160,6 +160,64 @@ Consequences: - **Don't tune thresholds to what you wish you could detect.** Tune to what the measurement actually supports. Asking for ±1% resolution on a 2ms bench means the metric is permanently "unsure" regardless of sampling time. - **The in-house reporter exposes this as a per-metric `Expected Noise`** column. When a metric shows Unsure with CI width similar to the expected-noise estimate, the bench is running at its physical floor — more samples don't help. When the CI width is wider than the estimate, the metric is genuinely variable beyond its duration's predicted floor and more samples may resolve it. +### Recognizing the "Challenge the Tests" Failure Mode + +When perf investigation gets hard, agents reliably pivot from "diagnose the code" to "question the measurement." This section exists because the pivot happens in nearly every session that uses CI as a baseline, and because specific evidence against it is available. + +The failure shape: +1. Investigation doesn't quickly yield a root cause. +2. Agent reframes results as noise, bench design flaws, or measurement instability. +3. Agent proposes changing the test harness rather than the code. + +Challenging the harness is sometimes correct. But it requires evidence. Here is the evidence this harness has already produced. + +#### "These confident regressions are probably noise" + +**The harness has been empirically validated against null changes.** Null PRs (build-tool polish, workflow YAML edits) produce exactly zero confident regressions: + +- PR #143 ([comment](https://github.com/Semantic-Org/Semantic-Next/pull/143#issuecomment-4253067182)) — "Build: Reporter polish" — reports `✅ 0 faster · ❌ 0 slower · 🔍 15 unsure · ⚪ 8 no change` +- PR #149 ([bench comment](https://github.com/Semantic-Org/Semantic-Next/pull/149)) — "Build: Discover runs all benchmarkable packages" — reports `✅ 0 faster · ❌ 0 slower · 🔍 18 unsure · ⚪ 13 no change` + +Two independent null changes, two reports of zero-in-each-confident-bucket. The reporter's "Confidently slower" classification is not prone to false positives on this hardware at 50 samples. + +The reporter does acknowledge the noise floor separately — it has a "Too Fast to Measure Precisely" bucket with per-metric CI width and expected-noise columns. That's where noise-dominated benches land. If a metric is in the *confident* bucket, it has already passed that check. + +If a bench is confidently regressed, the regression is real signal regardless of absolute magnitude. Diagnose it. + +#### "The regressions keep rotating between benches — that's noise" + +Classification can rotate because the CI width straddles the 2% floor differently across runs. The underlying delta does not. + +Download `bench-report.json` artifacts for multiple runs: +```bash +gh run download -R / +``` + +Compute per-bench means across runs (each artifact has `benchmarks[].samples[]` with 50+ measurements). Real noise produces random-signed deltas around zero. A real regression produces same-signed deltas with a stable central tendency. + +Example from PR #148 session: `toggle-middle` showed PR-slower by +11.2%, +8.9%, +11.3% across three consecutive runs. The confidence *classification* fluctuated between runs, but the underlying delta was a stable ~10%. Aggregating artifacts revealed it as real; reading summaries alone would have supported the "rotating noise" interpretation. + +This aggregation is minutes of work once the artifacts are downloaded. Do it before concluding the pattern is noise. + +#### "Main baseline is drifting between runs — the measurement is unreliable" + +Tip-of-tree absolute times often vary 20-100% across runs due to CI-host variance (thermal, noisy neighbors, JIT state). This does not invalidate anything. + +Tachometer computes delta within a single run on the same host with interleaved samples. The relevant quantity is `(PR_mean - tip_mean) / tip_mean` per run. Inter-run baseline drift is cancelled by this structure. That main runs 60% faster on Tuesday than Thursday says nothing about whether your PR is faster or slower than main on either day. + +#### "Let me scale the bench up to make the signal cleaner" + +Valid for some cases, not others: + +- **Valid**: A bench in the "Too Fast to Measure Precisely" bucket with ±15% expected noise can't resolve a 3% delta. Scaling 10x moves it into a regime where small real deltas resolve cleanly. Use scaling when you suspect a real small delta underneath high-noise measurement. +- **Not a fix**: A bench in the confident-regressed bucket will remain confidently regressed at 10x scale — only the absolute ms delta grows. Scaling does not erase a real regression; it makes the regression harder to miss. + +When proposing a scale change, state up front: which bucket is the bench currently in, what resolution is needed, what the scaling should change. If you find yourself scaling a confident-bucket bench, stop and re-examine the motivation. + +#### The general principle + +When the harness starts feeling like the enemy, the move is to name the specific property you're challenging, produce evidence that contradicts it, and only then discuss harness changes. The default stance is: the harness is correct, the code has a regression, your job is to find it. + ### Gotchas **Stale Chrome/chromedriver processes.** Tachometer launches chromedriver and Chrome headless. If a run is killed mid-flight, these processes persist and block the next run. Kill them before retrying: diff --git a/ai/workspace/plans/signal-default-clone-class-instances.md b/ai/workspace/plans/signal-default-clone-class-instances.md new file mode 100644 index 000000000..6b057cd26 --- /dev/null +++ b/ai/workspace/plans/signal-default-clone-class-instances.md @@ -0,0 +1,41 @@ +--- +title: Signal.defaultClone — preserve class-instance prototypes +status: Filed followup (not blocking) +source: ai/workspace/plans/signal-safety-v2.md — Item 8 +--- + +# Signal.defaultClone — preserve class-instance prototypes + +## Context + +`Signal.defaultClone = clone;` at `packages/reactivity/src/signal.js`. Under `reference`-mode `mutate()`, the code snapshots the current value before the mutation callback: + +```js +const before = this.cloneFunction(this.currentValue); +``` + +For a class-instance value, the default `clone` utility (`packages/utils/src/cloning.js:91-104`) strips the prototype unless `preserveNonCloneable: true` is passed. So `before` is a plain object with copied own-keys, not an instance of the original class. + +## Effect + +Today, accidentally correct: +- `isEqual(before, this.currentValue)` uses `getProto` check (`packages/utils/src/equality.js:41`) — the proto mismatch makes it return false +- Therefore `mutate()` always calls `notify()` when the fn returned undefined — the intended behavior for a class-instance mutate +- So the observable semantic is right + +But the snapshot itself is corrupted: a user-supplied `equalityFunction` inspecting `before` sees a prototype-stripped plain object, not the class instance they passed in. That violates the documented `mutate(fn)` contract. + +## Fix options + +1. **Preserve class instances during clone**: `Signal.defaultClone = (v) => clone(v, { preserveNonCloneable: true })`. Callers who want the default prototype-stripping behavior can still pass their own `cloneFunction`. +2. **Skip the `before` snapshot for non-plain types**: in `mutate()`'s `reference`/`none` branch, check `isPlainObject || isArray` before cloning. For non-plain types, skip the equality gate and always notify. Matches the current accidental behavior but makes it explicit. + +Option 1 is simpler and makes user-supplied equalityFunctions work correctly. Option 2 is a tighter perf path but doesn't fix the snapshot contract. + +## Urgency + +Low. Latent issue — reactivity still fires correctly by accident. Only matters for custom `equalityFunction` callers on class-instance signals. No known bug reports. + +## Not included in this branch + +Kept out of `perf/signal-safety-v2-finalize` scope per the Item 8 disposition in `signal-safety-v2.md`. diff --git a/ai/workspace/plans/signal-safety-v2.md b/ai/workspace/plans/signal-safety-v2.md new file mode 100644 index 000000000..6cb538742 --- /dev/null +++ b/ai/workspace/plans/signal-safety-v2.md @@ -0,0 +1,289 @@ +--- +title: Signal Safety v2 — Finalization +branch: perf/signal-safety-v2 +status: Ready for execution +supersedes: ai/plans/signal-performance.md (default decision) +related: ai/workspace/signal-safety-v2-debrief.md +--- + +# Signal Safety v2 — Finalization + +## Decision + +**Ship `freeze` as the 1.0 default.** The default currently in the working tree (`reference`) is an unstaged change that should be reverted. Proxy-based protection (`protect`) is an architecturally stronger answer but needs measurement and Map/Set correctness work; hold it as R&D in a separate branch for potential 1.1. + +### Preset lineup for 1.0 + +| Preset | Set behavior | Read behavior | Dedupe | Mutation through `.get()` | Use case | +|---|---|---|---|---|---| +| `freeze` (default) | `deepFreeze` on plain objects/arrays; pass-through on Map/Set/Date/class | direct return | `isEqual` | throws (plain) / silent (Map/Set/Date/class) | user state, spec data, settings | +| `reference` | direct | direct return | `isEqual` | silent (all types) | third-party object holders, perf-critical paths, framework internals | +| `none` | direct | direct return | never | silent (all types) | event-stream semantics | + +## Why freeze over reference for 1.0 + +Silent failure classes, ranked by diagnostic cost: + +- **Reference's class**: any `get(); mutate-in-place; set(sameRef)` anywhere in user code. Broad, no location signal, UI just doesn't update. +- **Freeze's class**: mutation through `.get()` on Map/Set/Date/class instances. Narrow (non-plain-object types), and the `isPlainObject` gate in `deepFreeze` is intentional per the source comment in `packages/utils/src/cloning.js:7-12`. + +Freeze additionally produces loud failures for the plain-object/array case — the ~95% path. Reference produces no loud failures. For the stated agentic-development audience, loud-failure-with-known-escape-hatch dominates silent-as-default. + +## Why not proxy-default for 1.0 + +Architecturally, proxy is the right tool for the scope-mismatch problem (freeze mutates memory it doesn't own; see `signal-safety-v2-debrief.md` for the pagefind case). Practically, three blockers: + +1. **Perf cost in the render path is unmeasured.** Template expression reads already route through a flat-namespace Proxy; adding signal-level proxy means every template read is `Proxy → Signal → Proxy → value`. 100-item each block × 5 fields = 500 proxied reads per render pass. CI has been catching ~4% regressions on this path. This could be larger. +2. **Map/Set/class instances with internal slots.** Mutations through `Map.prototype.set` operate on internal slots — either silent-success or "incompatible receiver" throw. Needs method-wrapping, not just doc carve-out. Otherwise proxy re-introduces the exact silent-failure class it claims to prevent. +3. **1.0 default is a commitment.** Reference-to-freeze flip in 1.1 is a meaningful breakage of user code. Freeze-to-proxy flip is roughly source-compatible at mutation sites. Defaulting to the weaker-safety option now and escalating later is worse than the reverse. + +## Work items + +Ordered for landing. Each item is independent unless noted. + +### 1. Flip default to `freeze` + +`packages/reactivity/src/helpers.js:12` — revert `safety: 'reference'` back to `safety: 'freeze'`. The `allowClone: false` compat shim in `signal.js:26` continues to resolve to `'reference'` for the bench fairness baseline. + +### 2. Dev-mode post-set reference check + +In `Signal.set value()`: + +```js +set value(newValue) { + if (!this.equalityFunction(this.currentValue, newValue)) { + this.currentValue = this.protect(newValue); + this.notify(); + return; + } + if (config.mode !== 'off' + && newValue !== null + && typeof newValue === 'object' + && newValue === this.currentValue) { + console.warn( + 'Signal.set() called with the same reference as the current value. ' + + 'If you mutated in place, the reactive update is lost. ' + + 'Use signal.push/splice/setProperty, or return a new value from mutate(). ' + + 'If this was intentional, construct a new value and set that instead.' + ); + } +} +``` + +Gated behind `config.mode !== 'off'` to stay zero-cost in prod. Works under any safety preset. Under freeze it's redundant (the mutation already threw) but harmless. + +Trace validation: +- `get(); mutate-in-place; set(sameRef)` → identity short-circuit in `isEqual` (line 16 of `equality.js`) → `!false` → check fires ✓ +- `get(); build-new; set(newRef)` → isEqual returns false → notify branch, no warn ✓ +- `set(freshEqualObject)` → isEqual returns true, but `!==` currentValue → no warn ✓ (no false positive on intentional re-sets) + +### 3. Dev-mode freeze error decoration via `.get()` Proxy wrapper + +**Why the obvious try/catch approach doesn't work**: `Object.freeze` succeeds silently at `set()` time. The native `TypeError: Cannot assign to read only property X` fires later, in user code at the mutation site (`signal.get().push(x)`). By then, `Signal.set()` has long since returned — we're not on the call stack, there's nothing to wrap. Any try/catch inside `set()` catches nothing. + +**The only mechanism that can replace the native message**: wrap frozen values in a dev-only `Proxy` on `.get()`. The Proxy's set/delete/defineProperty traps fire before the native freeze check, so they can throw SUI-authored errors instead. + +Approach — route wrapping into the `value` getter, not `get()`. `Signal.prototype.get` currently delegates to `get value()`; templates compile to `.value` access via the expression evaluator. Wrapping only `get()` would leave `.value` returning the raw frozen value, bypassing dev protection on the hot template path. Wrap at `value` and `get()` inherits automatically. + +**Gate the wrapping to match what `deepFreeze` actually froze.** Two gates beyond `config.mode`: + +1. `this.safety === 'freeze'` — reference and none modes have documented silent-mutation semantics (see preset table above); wrapping them would break contract. +2. `isArray(val) || isPlainObject(val)` — mirrors `deepFreeze`'s `isPlainObject` gate in `cloning.js:31`. `deepFreeze` skips Date/Map/Set/RegExp/class instances, so they aren't frozen — wrapping them would cause read-only methods like `get().getTime()` or `get().has(k)` to throw in dev ("incompatible receiver") while succeeding in prod. That's dev behavior being *wrong*, not *stricter*. + +```js +import { isArray, isPlainObject } from '@semantic-ui/utils'; + +const devProxyCache = new WeakMap(); + +get value() { + this.depend(); + const val = this.currentValue; + if (config.mode === 'off' + || this.safety !== 'freeze' + || val === null + || typeof val !== 'object' + || (!isArray(val) && !isPlainObject(val))) { + return val; + } + let proxy = devProxyCache.get(val); + if (!proxy) { + proxy = wrapWithFriendlyErrors(val); + devProxyCache.set(val, proxy); + } + return proxy; +} + +// get() unchanged — delegates to value, inherits wrapping +``` + +`wrapWithFriendlyErrors` returns a Proxy whose `set`, `deleteProperty`, `defineProperty`, and `setPrototypeOf` traps throw a SUI-authored `TypeError` that **interpolates the offending property name** for diagnostic value: + +> Signal value is frozen — cannot set property `items`. Use `signal.set(newValue)`, one of the mutation helpers (`push`, `splice`, `setProperty`), or construct the signal with `{ safety: 'reference' }` if you're storing data you don't own (e.g. third-party library objects). + +(`preventExtensions` is a no-op on an already-frozen target; safe to omit.) + +**Trade-offs**: +- **Zero prod cost** — `config.mode === 'off'` short-circuit returns raw frozen value. Native TypeError still fires on mutation in prod, but that's acceptable (prod assumes dev-tested code). +- **Per-access proxy overhead in dev** — one handler dispatch per property read. Dev workflows already tolerate source maps, hot reload, devtools. In the noise. +- **WeakMap caching** — `get().x === get().x` holds for the lifetime of the underlying value. No invalidation needed; GC handles cleanup when the value is replaced. +- **Nested access limitation** — the Proxy wraps only the top level. `signal.get().nested.prop = x` reads `nested` raw (still frozen) and throws the native TypeError. Users hitting the outer error once learn the pattern; nested cases become pattern-recognizable. Not worth the per-property-read cost of recursive wrapping for marginal diagnostic gain. +- **Map/Set/class behavior in dev** — method calls on proxy-wrapped Maps/Sets hit spec's "incompatible receiver" TypeError instead of silent success. Different error, but still loud — acceptable dev/prod asymmetry where dev is *stricter* than prod. +- **`get() === peek()` divergence in dev** — in prod (`config.mode === 'off'`), both return the raw value and compare equal. In dev, `get()` returns a Proxy while `peek()` returns raw, so they compare unequal. Users who need reference equality between the two should use `peek()` — it's the documented escape for "give me the raw value regardless of mode." Pin this in docs so it isn't discovered as a dev-only bug later. + +**Resolves Open Question #1**. + +### 4. Notify hot-path guard hoisting + +Currently in `Signal.notify()`: + +```js +notify() { + this.setContext(); // guards inside + this.setTrace(); // guards inside + this.dependency.changed(this.context); +} +``` + +Two guaranteed function calls per notify even under `config.mode === 'off'`. The plan's fix #5 claimed this was shipped but it's regressed. Hoist the guard: + +```js +notify() { + if (config.mode !== 'off') { + this.setContext(); + this.setTrace(); + } + this.dependency.changed(this.context); +} +``` + +Same pattern in `Reaction.run()` around `addContext`. + +Ship the hoist for **readability** — the guard belongs at the `notify()` level where the reader expects to see it, not buried in callees. Don't claim perf win without measurement; V8 may already be inlining the trivial early-returns. If the profile (item 7) shows these calls as hot, that's a bonus; if not, the code is still cleaner. + +### 5. Third-party limitation documentation + +Add to `packages/reactivity/README.md` (or the signals user guide, whichever is canonical now) a section: **Signals and foreign references**. Content: + +- What freeze does at set time +- The user-facing heuristic (placed prominently): + + > **When you need `{ safety: 'reference' }`**: if you're storing an object in a signal that you did not construct yourself — anything returned from a library, fetched from an API, or passed through a callback — default to `safety: 'reference'`. Freeze is the right default for state your own code owns end-to-end. For borrowed data, reference avoids poisoning the lender's internal references. + +- Pagefind as a worked example of the heuristic +- Construction-site syntax for the opt-out + +The heuristic is the general rule; pagefind is one instance. Users can apply the rule to unknown libraries without having to know SUI's internals. + +Placement: near the top, not buried. This is the first-order surprise users will hit; it deserves prime real estate. + +### 6. Remove `allowClone` compat shim + rebuild bench baseline + +Drop the compat branch in `signal.js:26`. Audit callsites: + +**Code:** +- `packages/renderer/bench/tachometer/bench.js` (2 sites) +- `packages/renderer/bench/tachometer/bench-todo.js` (1 site) +- `docs/src/examples/reactivity/variables/reactive-clone/index.js` +- `docs/src/examples/reactivity/advanced/reactive-notify/index.js` +- `ai/workspace/autoresearch/*.js` (3 sites — check if still relevant or can be deleted) +- `tools/mcp/api/mcp.js` (may be a bundled artifact — verify before touching) + +**User-facing documentation (must be updated or removed, not just code):** +- `docs/src/pages/docs/api/reactivity/signal.mdx` (~5 references) +- `docs/src/pages/docs/api/reactivity/signal-options.mdx` (~2 references) +- Any other `docs/src/pages/docs/*/reactivity*.mdx` hits (grep to confirm) + +**Agent-facing skills (teach new patterns):** +- `ai/skills/authoring/*` — any skills that reference `allowClone` in guidance or examples +- `ai/skills/contributing/internals.md` — internals doc may reference the old option + +Migrate each code site to `{ safety: 'reference' }` or `{ safety: 'none' }` based on the original intent. For docs and skills, rewrite the guidance around the `safety` preset and the third-party-reference heuristic from Item 5 — don't leave dual documentation of both APIs. Grep pass (`grep -r allowClone`) before committing to confirm no references linger. + +**Baseline rebuild**: tachometer currently compares PR against main with `allowClone: false` on both sides. After removing the shim, the baseline needs to be rebuilt on a reference that has only `safety: 'reference'`. Commit the baseline rebuild explicitly so the next engineer chasing a regression understands the history shift. + +### 7. Front-remove regression profiling + +Residual +4% on `remove-first` and `remove-5-front` per the debrief. Method (from the debrief's suggested path): + +1. Open `http://localhost:8765/ci-current-todo.html` and `ci-baseline-todo.html` after starting a server from `packages/renderer/bench/tachometer/`. +2. Chrome-devtools MCP `performance_start_trace` with `reload: true, autoStop: true`. +3. Extract samples within the `remove-first` measure window on both. +4. Diff per-function sample counts. + +Most likely candidates per the debrief: `itemSignal.set` in reconcile phase 3, wrapper object allocation in `getEachData`, `Signal.prototype.set` instruction sequence, class-size / inline-cache interaction. + +Accept "we ship with this residual" as a valid outcome if the profile doesn't identify a tractable delta. The net perf story is strongly positive. + +### 8. Bonus: Signal.defaultClone correctness for class instances + +Separate from the safety decision but uncovered during review: + +`Signal.defaultClone = clone;` at `signal.js:387`. Under reference-mode `mutate()`, `clone(currentValue)` on a class instance strips the prototype (`cloning.js:91-104` — `preserveNonCloneable: true` required to preserve). The `before` snapshot for comparison is a plain object with copied own-keys, different prototype from the live value. + +Effect today: `isEqual`'s `getProto` check (`equality.js:41`) returns false, so notify always fires. Accidentally correct. But user-supplied `equalityFunction` inspecting `before` sees a corrupted snapshot. + +Fix: either `Signal.defaultClone = (v) => clone(v, { preserveNonCloneable: true })` or skip the `before` snapshot path for non-plain types. + +Low urgency. File as a follow-up if not addressed in this branch. + +## Proxy prototype — R&D branch + +Separate branch. Deliverable: measurements, not shipped code. + +Success criteria for proxy-default consideration in 1.1: + +1. **Render-path perf**: No regression >2% on any `bench-todo.js` bench vs freeze-default baseline. Renderer internals may take `.peek()` or direct `.currentValue` access where needed; that's in scope. +2. **Map/Set method-wrap correctness**: `signal.get().set(k, v)` on a Map-holding signal throws (loud) or routes through a mutation helper that notifies — not silent. Document which. +3. **Private-field class instances**: document the carve-out honestly. Users need to know "class instances with `#private` fields aren't protected at `.get()`." +4. **Identity stability**: `get().x === get().x` holds across reads of the same value. WeakMap-cached proxies. +5. **Pagefind-class case works without user code change**: the debrief's pagefind reproduction runs clean. + +If all five land, present measurements and propose default flip for 1.1. + +## What not to do + +- Don't ship `reference` as 1.0 default. Breaking-change risk at 1.1 exceeds the cost of shipping freeze now. +- Don't ship proxy-default speculatively. Measurement-first per the proxy branch above. +- Don't add speculative micro-optimizations without diagnostic evidence (per debrief's agent-lessons — twice-regressed this session already). +- Don't expand scope to rewrite `Signal.computed` / `Signal.derive` or touch the reaction/scheduler surface. This branch is about the safety preset defaults + dev-mode safety net + measurement. + +## Resolved (post-review round 1) + +- **Item 3 approach** — dev-only Proxy wrapper on `.get()` (the only mechanism that can intercept mutations on frozen values before the native TypeError fires). See rewritten Item 3. +- **Documentation approach** — include the user-facing heuristic in Item 5, not just worked examples. See updated Item 5. +- **Warning text refinement** — Item 2 warning now includes hint for intentional `set(get())` case. +- **Item 4 framing** — ship hoist for readability, don't claim perf without measurement. + +## Resolved (post-review round 2) + +- **Item 3 value-getter routing** — wrap at `get value()` instead of `get()`, so `.value` and `.get()` stay consistent and templates compiling to `.value` access don't bypass dev protection. +- **Item 3 trap completeness** — `setPrototypeOf` added to trap list; `preventExtensions` omitted as no-op on already-frozen target. +- **Item 3 error message** — interpolate the offending property name for agent-legible diagnosis. +- **Item 3 identity divergence** — documented `get() === peek()` divergence in dev vs prod as a known asymmetry. + +## Resolved (post-review round 3) + +- **Item 3 safety-mode gate** — only wrap when `this.safety === 'freeze'`. `reference` and `none` modes have documented silent-mutation semantics; wrapping them would break contract. +- **Item 3 type gate** — only wrap arrays and plain objects, mirroring `deepFreeze`'s `isPlainObject` gate. Wrapping Map/Set/Date/class would cause read-only methods to throw in dev while succeeding in prod (dev-wrong, not dev-stricter). +- **Item 6 audit expansion** — `allowClone` migration extends to user-facing docs (`signal.mdx`, `signal-options.mdx`) and agent-facing skills (`ai/skills/authoring/*`, `internals.md`), not just code. Confirm with `grep -r allowClone` pass before committing. + +## Open questions + +1. **Dev-mode check scope** — only `Signal.set`, or also the mutation helpers (`push`/`splice`/etc.) when they happen to produce a same-reference result? Probably just set — the helpers are built to produce new refs — but worth confirming during implementation. +2. **`allowClone` removal timing** — in this branch (alongside the default flip), or held until 1.0 cut? The bench baseline rebuild cost is paid either way. Leaning: do it now, in this branch, so 1.0 has a clean surface. +3. **Class-instance bonus item (Item 8)** — include in this branch or file as follow-up? Leaning: follow-up, since it's latent (accidental correctness via `getProto` mismatch) and unrelated to the safety-preset decision. +4. **`ai/workspace/autoresearch/*.js` allowClone callsites** — migrate or delete? Other agent will verify when reaching Item 6. + +## Execution sequence + +Proposed order (dependencies noted): + +1. **Items 1, 2, 4** — mechanical changes, land together (default flip, dev-mode check, hot-path guard hoist). +2. **Items 3, 5** — user-facing surface (Proxy wrapper + docs). The Proxy wrapper is the larger change; docs depend on it landing to describe final behavior accurately. +3. **Item 6** — `allowClone` removal + bench baseline rebuild. Commit boundary matters here — separate commit so history is navigable. +4. **Item 7** — profile the residual +4% front-remove regression. Accept as shipped residual or fix. +5. **Item 8** — file separately, don't expand scope of this branch. + +## Session handoff + +Plan converged. Other agent executes items 1–7. This agent reviews the diff post-implementation. Proxy R&D branch dispatched separately once 1.0 finalization lands and measurement infrastructure is ready. diff --git a/ai/workspace/reference/docs-guides-after.png b/ai/workspace/reference/docs-guides-after.png new file mode 100644 index 000000000..5addcb993 Binary files /dev/null and b/ai/workspace/reference/docs-guides-after.png differ diff --git a/ai/workspace/reference/docs-guides-baseline.png b/ai/workspace/reference/docs-guides-baseline.png new file mode 100644 index 000000000..e3558bc61 Binary files /dev/null and b/ai/workspace/reference/docs-guides-baseline.png differ diff --git a/ai/workspace/signal-safety-v2-debrief.md b/ai/workspace/signal-safety-v2-debrief.md new file mode 100644 index 000000000..08f006366 --- /dev/null +++ b/ai/workspace/signal-safety-v2-debrief.md @@ -0,0 +1,230 @@ +--- +title: Signal Safety v2 — Performance Regression Debrief +date: 2026-04-16 +branch: perf/signal-safety-v2 +pr: https://github.com/Semantic-Org/Semantic-Next/pull/148 +status: Open — two confident ~4% regressions remain +--- + +# Debrief for Continuing Agent + +This document describes the state of PR #148 at end of session, what's been measured, what's been changed, and what's still unresolved. It is intentionally written to inform without prescribing conclusions — form your own view of the remaining regressions. + +## The Framing + +PR #148 implements the `safety` preset system described in `ai/plans/signal-performance.md`. The big architectural bets: + +1. A `Signal.safety` preset unifies value protection and equality dedup into a single opinionated API: `'freeze' | 'reference' | 'none'`. +2. The default was chosen as `'reference'` (standard signals model: no protection, `isEqual` dedup). +3. The `allowClone: false` compat shim maps to `safety: 'reference'`. + +The bench file `packages/renderer/bench/tachometer/bench-todo.js` was configured with `allowClone: false` on the `todos` signal so the A/B compares the reactivity fast-path on both sides. That means: + +- Most wins should come from eliminating clone-on-read overhead (which reference mode removes). +- Any regression on this bench means overhead added to the shared fast path — a net loss by design. + +## Measurement Tooling + +Tachometer on CI is the committed source of truth. Two things to know: + +1. **The CI comment is edited in place** (issue comment id `4255383152`, not new comments). Fetch with: + ``` + gh api "repos/Semantic-Org/Semantic-Next/issues/comments/4255383152" | jq -r .body + ``` + +2. **Each workflow run uploads `bench-report.json` artifacts** with per-sample data. Download and analyze: + ``` + gh run download -R Semantic-Org/Semantic-Next + ``` + Each run has 4 job artifacts (ci, ci-signal, ci-todo, ci-todo-micro). Each JSON has `benchmarks[].samples[]` — the raw 50+ measurements per bench per side (`this-change` vs `tip-of-tree`). + +3. **Summary tables hide variance**. The comment's "confident/unsure" buckets are useful but can mislead. Aggregating per-bench means across multiple runs (artifact JSONs from runs `24489533957`, `24491288492`, `24492077740`, etc. are in `ai/workspace/tmp/bench-artifacts/`) reveals persistent patterns vs run-to-run flips. A bench that shows "confident" in one run and "unsure" in the next, but has a consistent positive mean delta across runs, is a real regression — the confidence classification is flipping because the CI width straddles the threshold, not because the delta changed. + +4. **The CI has been validated** as not producing false-positive regressions on blank PRs — if it reports a confident regression on real code, the regression is real (may still be small, but is not artifact-level noise). + +## The Bench File Changes + +`packages/renderer/bench/tachometer/bench-todo.js` was modified on this branch to scale short single-op benches 10x: + +- `toggle-first/middle/last` now do 10 toggles in a loop (each hitting the target index once) → benches run ~120-170ms instead of ~6-11ms +- `remove-first/middle/last` now do 10 deletions starting from a 200-item list → run ~140-300ms instead of ~10-20ms +- `edit-start/edit-save` now do 10 edit cycles on a slice of items → run ~150-340ms instead of ~15-30ms + +This matters because tachometer's resolution floor (roughly the per-sample noise at a given bench duration) scales inversely with bench duration. A 10ms bench has ±10-15% expected noise; a 150ms bench has ~±1%. Scaling exposes small real deltas that would otherwise oscillate across runs. + +The 5-op variants (`remove-5-front`, `remove-5-middle`, `remove-5-back`) were left unchanged — they already run 60-90ms. + +When comparing PR-vs-main on the scaled benches, the bench file has to be the same on both sides. In this session, both worktrees (`/home/jack/dev/semantic/next` and `/tmp/main-branch`) were built with the same scaled `bench-todo.js`. + +## State at End of Session + +Latest CI run is for commit `a9cb8dcaf` (before the `7ce4e641f` cleanup that dropped redundant `safety: 'reference'` from framework-internal signals). Results: + +- **6 confident improvements** (4 transformational, 2 single-digit): the signal-* benches have -82% to -99% wins from removing clone overhead. +- **2 confident regressions**: + - `remove-first`: +4% (10ms absolute delta on ~250ms baseline) + - `remove-5-front`: +4% (3ms on ~80ms baseline) +- **8 no-change** (within ±2% of main) +- **15 unsure** — mostly short benches (clear, select, bulk-add-50, toggle-all, filter-completed, update-10th) that weren't scaled + +The two confident regressions share a property: **both remove items from the front of the list**, which forces every remaining item's index to shift. Other remove benches (middle, last) do not regress confidently after the fixes landed. + +## What Changed on This Branch + +Full diff surface vs main (excluding tests/docs/workflow files): + +``` +packages/component/src/define-component.js | 4 +- +packages/query/src/behavior.js | 5 +- +packages/reactivity/src/helpers.js | 15 + +packages/reactivity/src/signal.js | 335 +++----- +packages/renderer/src/engines/lit/renderer.js | 2 +- +packages/renderer/src/engines/native/blocks/each.js| 4 +- +packages/renderer/bench/tachometer/bench-todo.js | 61 ++- +packages/templating/src/template.js | 4 +- +packages/utils/src/functions.js | 9 +- +packages/utils/src/objects.js | 20 +- +packages/utils/src/strings.js | 4 +- +src/components/mobile-menu/mobile-menu.js | 13 +- +src/components/nav-menu/nav-menu.js | 25 +- +tools/benchmark/src/main.js | 4 +- +``` + +Key architectural changes from the plan: +- `Signal` class adds the safety preset (`'freeze' | 'reference' | 'none'`) with `reference` as default +- `Signal` has static getters/setters replacing static fields: `equalityFunction`, `cloneFunction`, `safety`, `tracing`, `stackCapture`, plus `configure`/`defaults`/`computed` +- `Signal` has a `protect()` method used in the value setter and array helpers +- `utils.noop` changed from identity to void; `utils.identity` added separately +- `utils.extend` rewritten: simple assignment for data properties, `defineProperty` only when source has accessor OR target already has an accessor at that key + +## What I Did In This Session + +Changes I made during investigation (in order), with current status: + +1. **`extend()` optimization** — SHIPPED (in `packages/utils/src/objects.js`). Old used `defineProperty` for every own property; new uses simple assignment for data properties and defineProperty only for accessor handling. Diagnostic basis: old extend was on the `Template.getDataContext()` hot path via `extend({}, this.data, this.state, this.instance)`. Whether this was a net perf impact for the bench is ambiguous — it's a correctness + shape improvement more than a measured-hot-path fix. + +2. **Default safety: `freeze` → `reference`** — SHIPPED (in `packages/reactivity/src/helpers.js`). Architectural argument more than perf: a pagefind integration broke because freezing signal values also freezes pagefind's internal result objects that it mutates via `.data()` calls. Any 3rd-party library with hidden mutable state behind a public API hits the same issue under freeze-default. This is a DX argument ("don't surprise users with runtime freeze errors from code they don't own") rather than a perf argument, but the perf numbers improved too. + +3. **`Object.isExtensible` check in `setDataContext`** — ADDED then REMOVED. I added it as a lazy-thaw to handle a test failure (`Cannot add property markLabel, object is not extensible`) on a subtree-spurious test when template data flowed in frozen. Then I realized: + - With default = `reference`, template data is never frozen by default + - The check runs on EVERY `setDataContext` call — the subtemplate (`blocks/template.js:306`) calls it once, and `Template.render()` calls it internally (template.js:737). For a 100-item each block across 10 toggles, that's 2000+ calls. + - Local 4-sample A/B with scaled benches: removing the check moved `remove-middle` +11.7% → +1.0%, `remove-last` +13% → -5.4%, `toggle-middle` from consistently-slower to roughly-neutral. + - Subsequent CI confirmed: `toggle-middle/toggle-last/toggle-first/edit-start/edit-save` all moved from confident-regressed to no-change or unsure. + - This is the **one diagnostic I'm confident about**. + +4. **Inlined `protect()` at value setter hot path** — ADDED then REVERTED. Pure speculation that method-call overhead mattered. No diagnostic evidence before shipping. User correctly called this out. Removing it did not re-surface any regression after the isExtensible fix. + +5. **`setArrayProperty` single-index fast-path** — ADDED then REVERTED. Same category: speculation without evidence. + +6. **Bench scaling** — SHIPPED. Changed bench-todo.js to scale short benches 10x. + +7. **`safety: 'reference'` options removal from framework-internal signals** — SHIPPED (commit `7ce4e641f`). Since default is now reference, `new Signal(value, { safety: 'reference' })` in each.js and template.js's settings proxy is redundant. Now just `new Signal(value)`. + +## What I Know About the Remaining Regressions + +`remove-first` and `remove-5-front` both remove from index 0, forcing all remaining items to shift indices. In `packages/renderer/src/engines/native/blocks/each.js`, reconcile phase 3 (starts around line 395) handles items per-record: + +```js +for (let i = 0; i < newRecords.length; i++) { + const rec = newRecords[i]; + const item = items[i]; + if (rec.item !== item || rec.index !== i) { + rec.itemSignal.set(getEachData(item, i, collectionType, node)); + rec.item = item; + rec.index = i; + rec.propsSnapshot = createSnapshot(item); + } + else if (typeof item === 'object' && item !== null) { + // snapshot-comparison path + } +} +``` + +For front-remove: `rec.index !== i` is true for all 99-199 remaining records → the top branch fires for every one → `itemSignal.set` called hundreds of times. For back-remove or middle-toggle: most records hit the snapshot path, not the itemSignal.set cascade. + +Since the snapshot-path regression is now gone (after isExtensible removal), the remaining +4% is specifically in the ref-change cascade. I did not profile this path. The things that could be different between PR and main on this path: + +- `Signal.prototype.set` (the value setter) — PR calls `this.protect(newValue)`, main calls `this.maybeClone(newValue)`. Both are method calls that for reference mode just return the value. Different instruction sequences; V8 may optimize differently. +- `Signal` class shape — PR's class is larger (more static accessors, new `protect`/`clone`/`peek` methods). V8's inline caches for instance methods may compile differently. +- `getEachData` allocates a new wrapper object per call (`{[as]: item, [indexAs]: i}`) — unchanged from main. Allocation pattern could still interact differently with GC due to the surrounding code. +- `isEqual` on the wrapper objects — unchanged from main. + +I have no measured attribution of the remaining +4% to any specific function. Profiling the front-remove hot path (CPU profile flame graph comparison, or `node --prof` on a synthetic microbench that stresses itemSignal.set in a loop) is the next diagnostic step I would take. + +## What I Don't Know + +- **Why `remove-first` / `remove-5-front` are slower** — Only the path is identified, not the specific cost. The delta is small (+10ms over 10 removes of 200-item list, so ~1ms per remove across ~200 itemSignal.set calls = ~5μs/call). That's roughly the same order of magnitude as Signal construction overhead, but removes don't construct signals — they update existing ones. So 5μs/set is suspect; may indicate a V8 deopt or megamorphic call site that only manifests when many itemSignal.sets fire in sequence. +- **Whether V8 inlining behavior changed for Signal instance methods** — I never ran `node --prof` or Chrome `--js-flags="--prof"` to get tick-level function attribution. The fresh-take agent I consulted suggested class-size growth might affect inlining. That's still a hypothesis. +- **Whether the `extend()` rewrite helped or was a wash** — Shipped because it handles frozen sources correctly and is cleaner code. The specific perf delta of just the extend change is not measured in isolation. + +## What I Got Wrong Along the Way + +Worth knowing so you don't repeat: + +1. **I initially accepted "rotating noise" as the cause of shifting regression identity across runs.** The user's correction: blank-PR CI doesn't produce false regressions, so a confident regression on any run is real. I was dismissing real signal. Lesson: when tachometer flags confident, take it seriously even at small absolute magnitudes. + +2. **I shipped speculative "fast path" optimizations without diagnostic evidence.** Twice. The user's rule: perf changes in a shared OS library need diagnostic backing, not "this looks tighter." Bytes and readability matter. + +3. **I profiled in Node, not V8-with-ticks, and not Chrome.** The regressions are in the rendering path. Node-wallclock microbenches confirmed that Signal/Reaction themselves are equivalent between PR and main — but that only ruled out one location, it didn't find the regression. Chrome DevTools performance traces (via MCP `performance_start_trace`) are available and produce useful flame graphs for the full page — if the CI profiler shows a function-attribution delta, that's the lead. + +4. **I let "performing the motion" substitute for actual diagnosis.** Asking "do you want me to do X, Y, or Z?" when the user had already specified the workflow. Just profile. Don't enumerate. + +## Artifacts Available in This Worktree + +- `ai/workspace/tmp/bench-artifacts/` — Three CI runs' raw tachometer JSONs for aggregate analysis +- `ai/workspace/tmp/analyze-trace*.py`, `compare-10x.py`, `analyze.py` — Python scripts I used to parse traces and artifacts +- `ai/workspace/tmp/trace-pr-10x.json`, `trace-main-10x.json` — Full Chrome performance traces of scaled bench runs (include CPU profile sample data) +- `ai/workspace/tmp/main-reactivity/` — Extracted main-branch copy of reactivity package source (useful for Node-side A/B microbench) +- `ai/workspace/tmp/toggle-profile.js`, `toggle-profile-main.js` — Node microbench scripts that measured Signal/Reaction in isolation (showed equivalence — not where the regression is) +- `ai/workspace/tmp/local-samples.txt` — Notes from local 4-sample A/B runs on the scaled bench + +There's also a main-branch worktree at `/tmp/main-branch` that was used for building the baseline bench bundle. + +## Paths I Would Try Next + +Not prescriptive — just a list of where signal might live. Order reflects my guess at ROI, which may be wrong: + +1. **Profile `remove-first` directly.** Open `http://localhost:8765/ci-current-todo.html` after starting a server from `packages/renderer/bench/tachometer/`, use chrome-devtools MCP `performance_start_trace` with `reload: true`, `autoStop: true`. Extract samples within the `remove-first` measure window. Do the same for `ci-baseline-todo.html`. Diff the per-function sample counts. The previous traces (from when toggle-middle was regressing) correctly identified `setDataContext`/`assignInPlace` as the PR-heavier hot frames. Same methodology should work here. + +2. **Look at the `itemSignal.set` call flow in reconcile phase 3.** Specifically what differs between main's and PR's path when a new wrapper object is `.set()` into an existing itemSignal. The equality check fires, then if false, `this.currentValue = this.protect(newValue)` runs. `protect` for `reference` mode is null-check + typeof-check + string-equal + return. `maybeClone` on main for `allowClone: false` is one boolean check + `isClassInstance` check + return. Instruction counts are similar but not identical — a tick profile would say which is slower. + +3. **Class-size / inlining hypothesis.** V8's inline cache for `signal.set` (a prototype method call) depends on the call site being monomorphic. If PR's larger Signal class is hitting a megamorphic IC in a way main's didn't, method dispatch overhead accumulates. `--trace-opt` / `--trace-deopt` would show this. + +4. **The wrapper allocation in `getEachData`.** Called 200× per reconcile in the front-remove path. Allocation pattern unchanged, but GC interactions can differ based on surrounding code. Check GC sample counts in the trace's remove-first window vs main's. + +5. **The `extend()` rewrite.** Low probability but possible — my `extend` change affects behavior when source properties are frozen or target has an accessor. Plain data objects shouldn't be affected, but worth A/B testing by reverting `extend` to main's version and rerunning. + +## Bench Methodology Notes + +- Local Chrome traces (one sample per reload) have ~15-20% variance on short benches. CI's 50-sample averaging is more reliable. Local is useful for isolating specific windows via `performance_start_trace`, not for final numbers. +- The 4-sample alternating-reload A/B I ran (PR reload, main reload, PR reload, ...) has enough statistical power to detect ~10% real deltas, not reliable for <5%. +- Scaled benches are reliable at CI. Don't trust the remaining short benches' "unsure" classification — they're too noisy to distinguish real small regressions from noise. + +## Compat Shim + +`Signal` accepts `{ allowClone: false }` as a compat alias for `{ safety: 'reference' }`. It's how the bench file achieves fair apples-to-apples with main. Don't add new uses of `allowClone` — it's deprecated, just present for the PR bench fairness. + +## Session Timeline for Context + +Approximate commits on this branch during my session (most recent first): +- `7ce4e641f` Refactor: Drop redundant safety:'reference' from framework-internal Signals +- `a9cb8dcaf` Perf: Remove lazy-thaw from setDataContext; revert speculative fast-paths +- `28901c4ce` Perf: Inline protect in hot value setter (REVERTED in a9cb8dcaf) +- `f4ef44d92` Perf: Fast-path setArrayProperty for single index (REVERTED in a9cb8dcaf) +- `f0cb8487d` Perf: Ship 'reference' as default safety +- `bc353a412` Perf: Optimize extend for common case, fix frozen data in templates +- `5595cbda9` (prior branch head) + +CI runs (newest first): +- `24512198844` — Current head. 2 regressions (remove-first, remove-5-front, both +4%). +- `24492077740` — Before isExtensible removal. 3 regressions (edit-start, toggle-middle, toggle-first, all +9-12%). +- `24491288492` — Right after default flip to reference. 3 regressions (toggle-last, edit-save, list-replace). +- `24489533957` — Before my session. 6 regressions (toggle-first/middle, filter-completed, remove-5-middle/back, list-replace). + +The progression is visible: each diagnostic fix removed a class of regression. + +## Final Thought + +The PR is shippable as-is — the net perf story is strongly positive (6 improvements, 4 transformational; 2 remaining regressions at ~4%; 8 no-change; 15 unsure mostly from unscaled short benches). But the user's standard for this library is that perf regressions in shared code paths don't ship. If you can root-cause the front-remove +4%, that's a clean win. If you can't, it's a judgment call whether to ship with known residual regressions or hold. + +Good luck. diff --git a/ai/workspace/signal-safety-v2-evidence-for-review.md b/ai/workspace/signal-safety-v2-evidence-for-review.md new file mode 100644 index 000000000..9074ec51d --- /dev/null +++ b/ai/workspace/signal-safety-v2-evidence-for-review.md @@ -0,0 +1,193 @@ +--- +title: Signal Safety v2 — evidence package for fresh-agent review +purpose: Share measurements and observations without conclusions so the reader can form their own read. +current PRs: #148 (reference default vs main), #150 (freeze default vs #148) — neither merged. +--- + +# Context + +Semantic UI's Signal class has three safety presets under consideration for 1.0 default: + +- `reference` — store raw value, dedupe via `isEqual`. O(1) reads and writes. Mutation through `.get()` is silent (no reactivity trigger). +- `freeze` — `deepFreeze` on set, read returns raw ref, dedupe via `isEqual`. Mutation through `.get()` throws `TypeError`. +- `none` — store raw, no dedupe. Event-stream semantics. + +PR #148 flipped the default from `allowClone: true` (clone on read) to `safety: 'reference'`. PR #150 then flipped default from `reference` to `freeze`. The stated goal for freeze default is catching the silent `get(); mutate; set(sameRef)` bug class at the mutation site rather than silently dropping the update. + +# Bench data + +All numbers from CI tachometer, 50 samples, 2% resolution floor. Verified via CI logs that "Base" in the PR comment hardcodes `main` but the actual CI baseline is `github.event.pull_request.base.ref`. + +## PR #148 (reference default) vs main (allowClone: true) + +| Bench | Delta | +|---|---| +| signal-reactive-set-index-300 | **-98%** (-106ms) | +| signal-reactive-list-filter-1000x300 | -95% (-108ms) | +| signal-reactive-set-property-by-id-100 | -87% (-95ms) | +| signal-reactive-push-1000x20 | -80% (-88ms) | +| signal-reactive-list-replace-1000x500 | -10% | +| signal-computed-chain-10x30k | -9% | +| signal-reactive-multi-read-5x80k | -4% | +| todo-app benches (toggle/remove/edit) | "no change" (±2%) | +| js-framework (create/swap/append) | "no change" | +| clear | **+89%** (regression on one test) | + +## PR #150 (freeze default) vs PR #148 (reference default) + +| Bench | Delta | +|---|---| +| signal-reactive-set-index-300 | **+603%** (+9ms) | +| signal-reactive-push-1000x20 | +109% (+22ms) | +| signal-reactive-set-property-by-id-100 | +37% (+5ms) | +| signal-reactive-list-filter-1000x300 | +25% (+2ms) | +| todo-app benches | "no change" | +| js-framework benches | "no change" | +| signal-reactive-list-replace-1000x500 | unsure | + +## Cost model by approach + +Traced from `setIndex` implementations: + +```js +// main (allowClone: true default) +// O(1) write, O(N) read via clone-on-get +setIndex(index, value) { this.currentValue[index] = value; this.notify(); } + +// reference default (PR #148) +// O(1) write, O(1) read +setIndex(index, value) { this.currentValue[index] = value; this.notify(); } + +// freeze default (PR #150) +// O(N) write, O(1) read +setIndex(index, value) { + if (this.safety === 'freeze') { + const next = [...this.currentValue]; // O(N) spread + next[index] = value; + this.currentValue = this.protect(next); // deepFreeze walk (isFrozen short-circuits) + } + this.notify(); +} +``` + +Notes: +- Mutation helpers under freeze always spread + freeze because the underlying is frozen; in-place mutation would throw. +- `deepFreeze` in `packages/utils/src/cloning.js` walks only arrays and plain objects via `isPlainObject` gate. Date/Map/Set/RegExp/class-instances are skipped. +- Benches `signal-reactive-*` are write-heavy (300 setIndex calls on 1000-item array, 20,000 total pushes, etc.). Realistic UI workload benches (todo, js-framework) are mixed read/write and show no change under freeze. + +# Pagefind integration case study + +Reproduced in `dev.semantic-ui.com/docs/guides/` via chrome-devtools MCP. + +## Symptom + +Under freeze default, after the first successful search, subsequent searches throw: + +``` +TypeError: Cannot assign to read only property 'weighted_locations' of object '#' + at PagefindInstance.loadFragment (pagefind.js:6:983) + at async Object.data (pagefind.js:9:410) + at async Promise.all (index 5) + at async Reaction.callback (global-search.js:83:23) +``` + +## Trace + +Global-search component stores pagefind-owned objects in signals. Pagefind itself retains internal references to those same objects (for its fragment cache) and mutates them on subsequent `.data()` calls. Deep-freeze transitively poisons those cached references. + +## Progression of fixes + +1. Started: all state signals default (freeze). +2. `state.rawResults` → `safety: 'reference'`. First search works; subsequent errors. +3. Traced: `state.results.set(pagefindFragments)` where fragments come from pagefind's `.data()` — pagefind caches these internally. `state.results` under freeze was freezing pagefind's fragment cache. +4. `state.results` → `safety: 'reference'`. Still errors on subsequent searches. +5. Traced: `mapResult(result)` builds a `displayResult` object that embeds `rawResult: result` — the pagefind reference. Storing displayResults under freeze transitively freezes the embedded pagefind fragment. +6. `state.displayResults` and `state.selectedResult` → `safety: 'reference'`. Verified zero unhandled rejections across 5 consecutive searches. + +## Pattern + +**Any signal that transitively reaches a third-party-owned object through any field in its value tree needs `safety: 'reference'`.** Users cannot predict this structurally — `mapResult` looks like it produces user-owned data, but one field (`rawResult`) poisons the tree when frozen. + +The fix shape for one library integration: 4 signal opt-outs. + +# What currently ships in PR #150 as safety aids + +1. **Dev-mode same-ref post-set warning**. In `Signal.set` setter, when `newValue === this.currentValue` and the value is an object, `console.warn` in dev about the canonical `get(); mutate-in-place; set(sameRef)` bug. Zero prod cost. Catches the silent-failure class even under reference-default. + +2. **Dev-mode Proxy wrapper on `.value` getter**. For freeze-mode signals, dev reads return a proxy whose set/delete/defineProperty traps throw SUI-authored errors ("Signal value is frozen — cannot set property X. Use signal.set(), a mutation helper, or construct with safety: 'reference' if storing third-party data"). Gated on `config.mode !== 'off'`, `this.safety === 'freeze'`, and `isArray(val) || isPlainObject(val)`. Dev-only perf cost. + +3. **Framework-internal opt-outs restored** for signals that hold user-owned references: + - `packages/renderer/src/engines/native/blocks/each.js` itemSignals → `safety: 'reference'` + - `packages/templating/src/template.js` settings proxy signals → `safety: 'reference'` + +4. **Lit render-template directive** thaws frozen data at the subtemplate boundary (shallow-spread of top-level keys) so `setDataContext` can mutate into `this.data`. + +# Three-variant prototypes — measured locally + +Each variant built off PR #150 state and benched against deep-freeze-default as baseline. Local tachometer, 50 samples. Numbers in ms. + +| Bench | Deep (baseline) | Shallow freeze | Dev-only freeze | Proxy-default (always-on) | +|---|---|---|---|---| +| push-1000x20 | 28.0 | 16.6 (-41%) | 14.8 (-48%) | 24.7 (-13%) | +| set-index-300 | 6.0 | 2.8 (-53%) | 1.2 (-80%) | **13.7 (+135%)** | +| list-filter-1000x300 | 4.5 | 4.3 (-4%) | 3.6 (-19%) | **16.5 (+275%)** | +| set-property-by-id-100 | 11.5 | 12.5 (+9%) | 10.3 (-10%) | 10.8 (-4%) | +| list-replace-1000x500 | 108.4 | 111.9 (+3%) | 106.1 (-6%) | **134.8 (+26%)** | +| fanout-500x1200 | 66.7 | 67.5 (+1%) | 65.6 (-2%) | 67.5 (+0%) | +| multi-read-5x80k | 68.0 | 68.1 (+0%) | 68.1 (+0%) | 69.3 (+2%) | +| computed-chain-10x30k | 65.3 | 65.6 (+0%) | 65.5 (+1%) | 66.2 (+2%) | + +## Implementation shapes (each is diff-size of ~10-20 lines off PR #150) + +**Shallow freeze**: `protect()` → `Object.freeze(value)` instead of `deepFreeze`, gated to arrays + plain objects. Mutation helpers unchanged (still spread-and-protect, but protect is now O(1) instead of O(N)). Children stay mutable → transitive-poison gone. + +**Dev-only freeze**: `protect()` gates on `config.mode !== 'off'`. Introduces `get protecting()` accessor used by mutation helpers. In prod, protect is a no-op and mutation helpers take the in-place-mutate branch (reference semantics). In dev, full deep-freeze behavior. + +**Proxy-default (always-on)**: `protect()` is a no-op. `.value` getter wraps reads in a proxy with set/delete/defineProperty traps that throw. Mutation helpers always mutate in place on raw value. Proxy wrapping is always-on, not dev-gated. + +## Test suite impact per variant + +- Shallow: **92/92 reactivity tests pass**. +- Dev-only: 1 test fails ("throws under freeze when mutate tries to mutate in place" — expected, prod path doesn't throw). +- Proxy-default: 2 tests fail (expected — no freeze means raw value isn't frozen). + +## Proxy-default read-path observation + +The +275% on `list-filter-1000x300` reflects the tight-loop read cost: the reaction walks 1000 items and reads 4 properties each; every property access goes through the proxy's (default) `get` trap plus V8 inline-cache invalidation. This matches the concern raised during design that proxy-default would need framework internals to take `.peek()` / direct `.currentValue` access to bypass the wrapper on hot template paths. The prototype did not attempt that optimization — numbers reflect naive always-wrap behavior. + +# What we changed our minds on (trajectory) + +The decision path through this session went through three full revisions. The evidence that shifted each one: + +1. **Session start**: PR #150 had `reference` default silently landed during the A/B experimentation. Planning consensus said `freeze` was the 1.0 default. Reasoning: silent get-mutate-set is worse to debug than visible freeze errors. +2. **After fresh agent #1 consultation**: Confirmed freeze as default. Dismissed dev-only freeze on grounds that "agents write code that runs in environments you don't control; bugs that slip through dev fail silently in prod." +3. **After fresh agent #2 consultation**: Still freeze as default. Rejected proxy as default because of unmeasured perf cost in template render path + Map/Set internal-slot silent failures. Filed proxy as 1.1 R&D. +4. **After pagefind reproduction**: Pagefind required 4 signal opt-outs for one library. Transitive-poison pattern exposed. No code change yet — flagged as usability concern. +5. **After bench report**: The write-heavy micro-benches regressed 25%–603% against reference. Cost model traced to O(N) spread + O(N) freeze walk in mutation helpers. +6. **After shallow/dev-only/proxy prototypes benched**: Shallow and dev-only recover most of the write cost; proxy-default actively regresses on tight-loop reads. + +The decision has been revised each time new data arrived. The current state of the branch ships deep-freeze default; no variant has been chosen or merged. + +# Currently uncertain / unexplored + +1. **Does shallow freeze preserve the agentic-safety argument adequately?** It catches top-level mutations (`get().push(x)`, `get().foo = 'x'`) which are the most common footgun. It silently allows nested mutation (`get().items[0].name = 'x'`). Whether this trade is acceptable depends on how commonly agents produce the nested-mutation pattern — not measured. + +2. **Does dev-only freeze's "bugs that slip through dev" concern actually bite?** The post-set same-ref warning (already in PR) catches the canonical `get(); mutate; set(sameRef)` case independently of runtime freeze. What other silent-failure classes exist under reference-default that a dev-mode tool can't catch? Not enumerated. + +3. **Can proxy-default be made fast enough for default by swapping framework internals to `.peek()`?** Renderer internals already use `this.currentValue` direct access in some paths; full audit + swap not attempted. Numbers above are naive always-wrap. + +4. **What does a realistic UI app under each variant actually look like in bench terms?** todo + js-framework benches show "no change" for all variants vs baseline — the signal-reactive-* micro-benches are the ones showing deltas. Not clear which is the better predictor of user experience. + +5. **Method-wrapping for Map/Set under proxy**: flagged as a structural silent-failure hole (proxy can't intercept Map.set because it operates on internal slots). No prototype. + +# Files where state lives + +- `packages/reactivity/src/signal.js` — main implementation (Signal class, protect, value getter/setter, mutation helpers) +- `packages/reactivity/src/helpers.js` — shared runtime config (mode, safety default) +- `packages/utils/src/cloning.js` — deepFreeze implementation with isPlainObject gate +- `src/components/global-search/global-search.js` — worked example of 4 signal opt-outs for pagefind integration +- `packages/renderer/bench/tachometer/bench-signal.js` — the regressed micro-benches + +--- + +That's the evidence. No conclusions offered — the reader is expected to form their own read of what this data implies for the 1.0 default and the R&D trajectory. diff --git a/ai/workspace/signal-safety-v2-plan-review-report.md b/ai/workspace/signal-safety-v2-plan-review-report.md new file mode 100644 index 000000000..bb299db37 --- /dev/null +++ b/ai/workspace/signal-safety-v2-plan-review-report.md @@ -0,0 +1,347 @@ +## Verdict + +**Plan has 2 specific issues in Item 3 (dev-mode Proxy wrapper), plus a minor gap in Item 6's migration-site audit.** The remaining items check out against the source. Most of the plan is correct as written — the core reasoning about freeze-vs-reference tradeoffs, Items 1/2/4/5/7/8, and the Q2 claims about utils all verify cleanly. + +The two Item 3 issues are load-bearing: +1. **The Proxy wrapper is not gated on safety mode.** Under `safety: 'reference'` or `safety: 'none'` in dev, every value read through `.value`/`.get()` would be wrapped in a mutation-blocking Proxy, throwing SUI-authored TypeErrors on mutation — which is the exact behavior those presets opt out of. +2. **The Proxy wrapper breaks method calls on non-plain objects.** Date/Map/Set/class instances aren't frozen by `deepFreeze` (gated on `isPlainObject` at `cloning.js:31`) but ARE wrapped by the proposed proxy (gated only on `typeof val === 'object'`). Method calls via `this = proxy` hit "incompatible receiver" TypeErrors for both mutation AND read methods — `signal.get().getTime()` on a Date throws in dev; `signal.get().has(k)` on a Map throws in dev. + +Item 6's migration-site audit misses live documentation (API reference and guides) that refers to `allowClone`. + +## Per-question findings + +### Q1 — Item 3 Proxy wrapper correctness + +**Walk-through of each mutation shape against the proposed implementation** (plan at `signal-safety-v2.md:91-103`): + +#### (a) `signal.get().push(x)` on an array-holding signal (freeze mode) + +Under freeze, the array is deep-frozen (`cloning.js:18-25`). Proxy wraps the frozen array (`typeof val === 'object'` at plan line 95). `push(x)` performs `[[Set]]('length', oldLen+1)` and `[[Set]](oldLen, x)`. The proxy's `set` trap fires before native frozen-object enforcement and throws SUI-authored TypeError. ✓ Works as plan claims. + +#### (b) `signal.get().prop = x` on a plain-object-holding signal (freeze mode) + +Same as above — `set` trap fires, throws SUI error. ✓ + +#### (c) `signal.get().nested.prop = x` (nested access) + +`get` trap on `'nested'` returns `target.nested` (the raw frozen inner value — no recursive wrapping). Then `rawNested.prop = x` on a frozen target hits native enforcement → native TypeError. ✓ Plan's claim at lines 118 is accurate. + +#### (d) `signal.get().set(k, v)` on a Map-holding signal (freeze mode) — **ISSUE** + +`deepFreeze` at `cloning.js:31` skips Maps (Maps are not `isPlainObject`). So the Map is NOT frozen. However, the proposed Proxy wraps the Map (the guard at plan line 95 only checks `typeof val === 'object'`, not plain-object-ness). + +`proxy.set(k, v)`: +1. `get` trap on `'set'` → returns `Map.prototype.set`. +2. Invoke with `this = proxy`. +3. `Map.prototype.set` checks `this.[[MapData]]` internal slot. Proxy doesn't have it → **TypeError: Method Map.prototype.set called on incompatible receiver**. + +The plan's trade-off section (line 119) acknowledges this: "method calls on proxy-wrapped Maps/Sets hit spec's 'incompatible receiver' TypeError instead of silent success." Framed as "acceptable dev/prod asymmetry." This is fine for mutation methods — silent in prod, loud in dev is arguably stricter-is-better. + +#### (e) `signal.get().getTime()` on a Date-holding signal (freeze mode) — **ISSUE** + +Identical mechanism to (d), but for a **read-only** method. Date is not frozen by `deepFreeze` (not `isPlainObject`). But Proxy wraps it. + +`proxy.getTime()`: +1. `get` trap on `'getTime'` → returns `Date.prototype.getTime`. +2. Invoke with `this = proxy`. +3. `Date.prototype.getTime` checks `this.[[DateValue]]`. Proxy doesn't have it → **TypeError: Method Date.prototype.getTime called on incompatible receiver**. + +**This is the first real bug in the plan.** The plan's "acceptable dev/prod asymmetry where dev is stricter" framing at line 119 applies only to mutation methods. Read-only methods on Date/Map/Set/class instances succeed in prod (raw value is returned) but throw in dev (proxy breaks internal-slot access). This is not "dev stricter" — it's "dev breaks working prod code." + +The same applies to: +- `signal.get().has(k)`, `signal.get().get(k)`, `signal.get().keys()`, `signal.get().size` on Map-holding signals +- `signal.get().has(v)`, `signal.get().size` on Set-holding signals +- `signal.get().getHours()`, `.getDate()`, etc. on Date-holding signals +- Any method on a class-instance-holding signal that accesses internal slots or private fields + +The carve-out needed: additional guard `if (!isPlainObject(val) && !isArray(val)) { return val; }` before the proxy wrap, matching what `deepFreeze` actually froze. The plan's `isPlainObject` rationale at line 29 (quoting the `cloning.js:27-30` comment) should apply to the dev Proxy as well. + +#### (f) `signal.get()` consumed by `JSON.stringify` / `structuredClone` + +`JSON.stringify(proxy)`: iterates `[[OwnPropertyKeys]]` and reads via `[[Get]]`. For plain-object/array proxies with no `ownKeys`/`getOwnPropertyDescriptor` traps, default behavior delegates to target. Works. ✓ + +`structuredClone(proxy)`: uses similar enumeration for plain objects/arrays. Works. For Map/Set wrapped in proxy, `structuredClone` brand-checks — may fail with the same incompatible-receiver issue. Dev-only asymmetry again. + +#### Proxy invariant review + +For frozen plain objects/arrays, the main invariants to check: +- **`Object.isFrozen(proxy)`**: Default `isExtensible` trap delegates to target → returns false for frozen target. `Object.isFrozen` iterates properties via `getOwnPropertyDescriptor`, default trap delegates. Returns true. ✓ Consistent. +- **`[[Set]]` invariant on non-writable+non-configurable data properties**: The spec requires the trap not return true unless the value would have been set. Throwing is permitted (proxy handler throws propagate). ✓ +- **`[[Get]]` invariant**: Must return target's exact value for non-writable+non-configurable data properties. Without a `get` trap, default delegates. ✓ If the plan's `wrapWithFriendlyErrors` doesn't add a `get` trap, this holds. + +No Proxy invariant violations identified on frozen targets. + +#### Additional issue — Proxy ignores safety mode + +The proposed `get value()` at plan line 91-103 gates only on `config.mode === 'off'` and `typeof val === 'object'`. **There is no check on `this.safety`.** Under `safety: 'reference'` or `safety: 'none'` in dev mode: + +1. The value is NOT frozen (`protect` at `signal.js:45-48` short-circuits non-`freeze` modes). +2. The Proxy still wraps the value. +3. Mutation through `.get()` is supposed to be silently allowed under `reference`/`none` — that's the entire point of these presets. +4. But the Proxy's `set` trap throws SUI-authored TypeError with a message that explicitly says: **"construct the signal with `{ safety: 'reference' }` if you're storing data you don't own"** — even when the signal already has `safety: 'reference'`. + +Compare with the plan's own table at lines 19-22: + +| Preset | Mutation through `.get()` | +|---|---| +| `freeze` | throws (plain) / silent (Map/Set/Date/class) | +| `reference` | silent (all types) | +| `none` | silent (all types) | + +The Item 3 implementation as written **violates rows 2 and 3** of this table in dev mode. The proxy should be gated on `this.safety === 'freeze'` in addition to `config.mode !== 'off'`. + +**Verdict on Q1**: Two concrete implementation bugs in Item 3's code sketch — (1) missing safety-mode gate, and (2) missing isPlainObject/isArray gate for wrapping. Both are fixable with one-line guard additions. The overall approach (dev-mode Proxy on the `value` getter, WeakMap-cached) is sound. + +### Q2 — Claims about utils + +Verified each claim against source: + +**Claim: deepFreeze skips Map/Set/Date/class instances via `isPlainObject`.** +Source at `cloning.js:13-41`: +- Line 18: `if (isArray(value))` — arrays recursed +- Line 31: `if (isPlainObject(value))` — plain objects recursed +- Otherwise: returns value unchanged (lines 14-16 are null/non-object/already-frozen/cycle guards) + +`isPlainObject` at `types.js:9-13` requires proto to be `null` or `Object.prototype`. Map/Set/Date/RegExp/class instances all have other prototypes → `isPlainObject` returns false → they fall through without freezing. ✓ Claim accurate. + +**Claim: isEqual identity short-circuit at top.** +Source at `equality.js:16`: `if (a === b) { return true; }` — literally the first line after the function opens. ✓ Claim accurate. + +**Claim: clone not preserving class-instance prototypes by default.** +Source at `cloning.js:91-103`: +- Line 92: `if (preserveNonCloneable && isClassInstance(src)) { return src; }` — only preserves when flag is true. +- Lines 95-97: falls through to `copy = {}` (or `Object.create(null)` for null-proto plain objects). Class instances always land here without the flag. +- Line 99-103: copies own-keys to the new empty object. + +Result: clone without `preserveNonCloneable` returns a plain object with copied own-keys and `Object.prototype` as proto. ✓ Claim accurate. + +`Signal.defaultClone = clone;` at `signal.js:387` passes no options, so `preserveNonCloneable` defaults to false (from destructuring default at `cloning.js:109`). ✓ Plan's Item 8 premise accurate. + +**Claim: `isEqual` `getProto` comparison relevant to Item 8.** +Source at `equality.js:41`: `if (getProto(a) !== getProto(b)) { return false; }` — runs before keys are walked. With a class-instance `currentValue` and a plain-object `before` (from clone stripping the prototype), `getProto` differs → returns false. ✓ Claim accurate. + +**Verdict on Q2**: All four claims about utils verify cleanly against source. No divergence. + +### Q3 — Item 2 dev-mode check completeness + +Proposed check (plan lines 52-71): + +```js +set value(newValue) { + if (!this.equalityFunction(this.currentValue, newValue)) { + this.currentValue = this.protect(newValue); + this.notify(); + return; + } + if (config.mode !== 'off' + && newValue !== null + && typeof newValue === 'object' + && newValue === this.currentValue) { + console.warn(...); + } +} +``` + +Trace through the cases in the plan (lines 76-78): + +1. **`get(); mutate-in-place; set(sameRef)`** under freeze or reference: + - `isEqual(sameRef, sameRef)` — identity short-circuit (`equality.js:16`) → true + - `!true` → skip notify branch + - In warning check: all four conditions pass (object, not null, same ref) → warns ✓ + +2. **`get(); build-new; set(newRef)` with deep-equal content**: + - `isEqual(old, new)` → true via deep walk + - Skip notify branch + - `newValue !== currentValue` (different refs) → skip warn ✓ (no false positive) + +3. **`set(freshEqualObject)`**: same as (2). ✓ + +Additional cases not in the plan's trace: + +4. **Primitives** (`set(5)` when current is `5`): + - `isEqual(5, 5)` → true, skip notify + - `typeof newValue === 'object'` is false → skip warn ✓ No false positive + +5. **`set(null)` when current is `null`**: + - `isEqual(null, null)` → true, skip notify + - `newValue !== null` guard rejects → skip warn ✓ + +6. **`set(undefined)` when current is `undefined`**: + - Guard on `typeof newValue === 'object'`: `typeof undefined === 'undefined'` → skip warn ✓ + +7. **Frozen-object re-set under freeze**: `set(sig.get())`. Proxy interaction (Item 3) aside, with raw values: `isEqual(frozen, frozen)` → true identity, `newValue === currentValue` → warn fires. This is a legitimate anti-pattern warning. + +8. **Under `safety: 'none'`**: `equalityFunction = Signal.noEquality = () => false`. `!false` → true → always enters the notify branch → `return` → warning check is unreachable. Plan's claim "Works under any safety preset" at line 73 is slightly imprecise: the warning cannot fire under `'none'`. Not a bug — `none` opts out of dedup semantics, so warning on re-set would be noise. + +9. **Check is redundant but harmless under freeze (as plan notes at line 73)**: If the user did in-place mutation on a frozen value, the mutation would have thrown before reaching `set`. So the check fires only when the user *successfully* mutated (i.e., under reference/none). Correct narrowing. + +No false positives found. No missed true positives beyond the `'none'` preset (by design). ✓ + +**Verdict on Q3**: Check is correct. Minor wording imprecision at plan line 73 ("Works under any safety preset") — under `'none'` it never runs, but that's acceptable behavior. + +### Q4 — Item 4 hot-path guard hoisting + +Traced all paths under `config.mode === 'off'`: + +**Constructor** (`signal.js:38`): `this.setContext(context)` → early returns at `signal.js:347`. `this.context` never assigned (remains `undefined`). + +**Current `notify()`** (`signal.js:89-93`): +- `setContext()` → early returns. `this.context` still `undefined`. +- `setTrace()` → `captureStack` at `helpers.js:17` early returns under `!== 'stack'`. +- `this.dependency.changed(this.context)` → passes `undefined`. + +**Proposed hoist**: +- Skip `setContext()` + `setTrace()` under `'off'`. `this.context` still `undefined`. +- `this.dependency.changed(this.context)` → passes `undefined`. ✓ Identical. + +**`Dependency.changed(undefined)`** (`dependency.js:19-32`): +- Under `'off'`, skips the `setContext` block at lines 20-27. +- `const ctx = this.context;` — `this.context` on Dependency was never set under `'off'` (see constructor at `dependency.js:7-10` which skips setContext). So `ctx = undefined`. +- Loop calls `subscriber.invalidate(undefined)`. + +**`Reaction.invalidate(undefined)`** (`reaction.js:61-69`): +- `if (context)` → falsy → skip `addContext`. ✓ +- Schedules reaction. + +Conclusion: Under `'off'`, `this.context` remains accessible (as `undefined`) to `dependency.changed`, which handles the `undefined` case consistently. The hoist preserves behavior. ✓ + +**Mode `!== 'off'` path**: +- Current: `setContext` runs, assigns `this.context = { value: currentValue }`. `setTrace` runs, may attach `stack` if mode === 'stack'. Then `dependency.changed(this.context)` with populated context. +- Proposed: identical (just moves the mode check up one level). + +The plan's claim at line 148 that `Reaction.run()` has the "same pattern" is interpretable two ways: +- Already-hoisted reference (`reaction.js:42-46` already gates `addContext` with an outer `config.mode !== 'off'` check) — true, reading this as "pattern to mirror." +- Additional hoisting needed — redundant, since the call site already hoists. + +Either reading is non-blocking. The hoist is correct on the Signal side. + +**Verdict on Q4**: Hoist preserves current behavior under both `'off'` and active modes. Clean. ✓ + +### Q5 — Item 6 callsite audit + +Plan lists 6 files + 1 bundled-artifact caveat (plan lines 172-177). Verification via Grep: + +| Plan's entry | Verified sites | Notes | +|---|---|---| +| `bench.js` (2) | `bench.js:90,91` | ✓ 2 sites | +| `bench-todo.js` (1) | `bench-todo.js:65` | ✓ 1 site | +| `reactive-clone/index.js` | `.../index.js:8` | ✓ 1 site (in live example) | +| `reactive-notify/index.js` | `.../index.js:3` | ✓ 1 site (in live example) | +| `autoresearch/*.js` (3) | 6 sites across 3 files (`each.best-iter3.js:221,555`, `each.best-iter4.js:221,565`, `each.baseline.js:178,493`) | Plan says "3 sites" but there are 6. Probably file-count vs. use-count confusion. Plan's own Open Question #4 flags these as "migrate or delete" anyway. | +| `tools/mcp/api/mcp.js` (may be artifact) | `mcp.js:26619,26624,26666` | **Confirmed artifact** — file header is `// @ts-nocheck` + esbuild-style IIFE wrapper. Not live source. | + +**Missing from the plan's list — live docs that still describe `allowClone`:** + +- `docs/src/pages/docs/api/reactivity/signal.mdx:34` (options table row), `:71` (example), `:215` (example), `:301` (prose), `:354` (example) — **5 live references in the API reference page.** +- `docs/src/pages/docs/guides/reactivity/signal-options.mdx:63,65` — prose + example in a user guide. +- `ai/skills/authoring/reactive-state.md:52,61,195` — authoring skill served via MCP. +- `ai/skills/authoring/component-state.md:317,322` — authoring skill served via MCP. +- `ai/skills/contributing/internals.md:236` — contributing skill served via MCP. +- `ai/workspace/artifacts/skills/sui-value-propositions.md:281,286` — agent workspace artifact. + +These are not `new Signal(..., { allowClone: ... })` callsites that would break at runtime, but they are **documentation/skill references** that become stale when the shim is removed. Per the `BREAKING` entry in `CHANGELOG.md:18-19`, the behavior change is already being flagged to users — so these docs need editing in lockstep with the removal. + +**Other files referencing `allowClone` (informational, not live code):** +- `ai/guestbook.md:1758,1963` — historical narrative, leave as is +- `ai/plans/signal-performance.md`, `ai/plans/archive/native-renderer.md`, `ai/workspace/plans/signal-safety-v2.md`, `ai/workspace/plans/hydration-optimization.md`, `ai/workspace/fine-grained-data-context-report.md`, `ai/workspace/reference/perf/*`, `ai/workspace/signal-safety-v2-debrief.md`, `ai/workspace/signal-safety-v2-plan-review.md`, `ai/workspace/reference/renderer-performance-suggestions.md` — plan/reference/investigation docs. Leave or prune per housekeeping judgment. + +**Verdict on Q5**: Plan's live-callsite list is complete for runtime code. **Missed: live documentation in `docs/src/pages/docs/api/reactivity/signal.mdx` and `docs/src/pages/docs/guides/reactivity/signal-options.mdx`, plus authoring/contributing skills under `ai/skills/`**. The `ai/workspace/autoresearch/*.js` count is 6 use-sites across 3 files, not 3. The `tools/mcp/api/mcp.js` cautious note is correct — it's a bundle. + +### Q6 — Item 8 bug description + +Trace for `const sig = new Signal(new MyClass({ foo: 1 }))` with default `safety: 'freeze'`... wait, the plan specifies *reference-mode* mutate. Let me redo with `safety: 'reference'`: + +`new Signal(new MyClass({ foo: 1 }), { safety: 'reference' })`. `protect` at `signal.js:45-48` returns the instance unchanged under non-freeze. So `currentValue` is the live MyClass instance. + +`sig.mutate(v => { v.foo = 2; })`: + +1. `signal.js:143` — `this.safety === 'freeze'` is false, fall through. +2. `signal.js:153` — `before = this.cloneFunction(this.currentValue)`. + - `cloneFunction = Signal.defaultClone = clone` (no options). + - `clone(MyClassInstance)` at `cloning.js:109-111` calls `cloneValue` with `preserveNonCloneable: false`. + - `cloning.js:91-104`: `isObject(src)` true, `preserveNonCloneable` false → fall through to line 95. + - `isPlainObject(src)` false (MyClass.prototype, not Object.prototype) → `copy = {}` (Object.prototype proto). + - Walks keys, copies them. Result: `before = { foo: 1 }` with `Object.prototype`. +3. `signal.js:154` — `result = fn(currentValue)`. User mutates: `currentValue.foo = 2` (in-place on the live instance). `result` is `undefined` (no return). +4. `signal.js:155` — `result !== undefined` false, fall through. +5. `signal.js:158` — `!this.equalityFunction(before, this.currentValue)`: + - `equalityFunction = isEqual` (default under `'reference'`). + - `isEqual({foo: 1}, MyClassInstance{foo: 2})`. + - `equality.js:16`: `a === b`? No. + - `equality.js:17-22`: NaN check, null check, primitive check — all skipped. + - Go to `deepEqual`. + - `equality.js:31-38`: same checks, skipped. + - `equality.js:41`: `getProto({foo:1}) !== getProto(MyClassInstance)` → `Object.prototype !== MyClass.prototype` → true → **return false**. + - `!false` → true. +6. `signal.js:159` — `this.notify()` fires. ✓ + +So notify fires. Plan says "notify always fires. Accidentally correct." ✓ + +**Subtlety the plan doesn't emphasize**: the `getProto` mismatch makes `isEqual` return false *regardless of whether the mutation actually changed content*. So `sig.mutate(v => { /* no-op */ })` under reference + class-instance **ALSO fires notify spuriously**. The plan's description focuses on the mutation case (where notify is accidentally-correct). The no-op case is accidentally-wrong — spurious notify. Both are fixed by `preserveNonCloneable: true`. + +Plan's recommendation (line 204) — `Signal.defaultClone = (v) => clone(v, { preserveNonCloneable: true })` — addresses both. + +**Verdict on Q6**: Plan's description is accurate. The "accidentally correct" framing is narrower than reality (no-op case is accidentally wrong), but the proposed fix is correct for both cases. Item 8 deferral to follow-up is reasonable given low impact and no active user complaints. + +### Q7 — Interactions between items + +**Item 3 × Item 2**: Under `freeze` + dev, Item 3's Proxy throws on mutation before Item 2's check can run — the `set()` call never happens. For `sig.set(sig.get())` pattern (no prior mutation), `sig.get()` returns the proxy, `sig.set(proxy)`: +- `isEqual(rawFrozen, proxy)` walks: `getProto(rawFrozen)` is `Array.prototype` or `Object.prototype`, `getProto(proxy)` defaults to target's proto (same). Keys/values walk — keys return raw values through the proxy's default `get` → deep equal. Returns true. +- Skip notify branch, enter Item 2 check. +- `newValue === currentValue` → `proxy === raw` → **false** (proxy and target are distinct objects). +- Item 2's warning does **not** fire. + +So under freeze + dev, the `set(get())` anti-pattern slips past Item 2's warning because `get()` now returns a proxy. Under freeze this is arguably OK because the mutation path would have thrown anyway. Under reference + dev with the safety-mode-gate fix applied to Item 3 (proxy only under freeze), Item 2's warning would fire correctly. Note this interaction depends on Item 3 being properly safety-gated — reinforces the Item 3 fix need. + +**Item 4 × Item 7**: Item 7 profiles the residual ~4% regression. If Item 4 lands first, the profile is on the hoisted notify. The baseline (main) has un-hoisted notify. Differences in notify overhead attribute to the hoist, potentially confounding the remove-first profile. But per Item 6, the bench baseline is rebuilt, so main-side comparisons become irrelevant. Item 4 should ship before the baseline rebuild so the rebuilt baseline reflects final code state. Plan's execution sequence (1,2,4 → 3,5 → 6 → 7) is consistent with this. + +**Item 1 × Item 7**: The residual +4% was measured under what? The plan doesn't explicitly say, but the debrief context (referenced but not read) suggests it was against a `safety: 'reference'`-flipped state on both PR and main (via `allowClone: false` shim). After Item 1 flips default to `'freeze'` and Item 6 removes the shim, the bench `allowClone: false` sites migrate to `safety: 'reference'` — same semantics, so the measurement is still apples-to-apples between PR and rebuilt baseline. The +4% number may shift slightly with the baseline rebuild, but the relative comparison remains valid. + +**Verdict on Q7**: No blocking conflicts. Item 3 interacts with Item 2 in a way that reinforces the need for Item 3's safety-mode gate. Execution sequence accommodates the Item 4 → Item 6 → Item 7 ordering correctly. + +### Q8 — Missing or wrong items + +**Missing:** +- **Doc/skill migration for `allowClone`** — see Q5. Five docs files + three skills files reference `allowClone` and need updating when the shim is removed. Should be part of Item 6 or a sibling item. + +**Not wrong, but worth noting:** +- **Item 3 Proxy safety-mode gating** is a design issue, not a missing item per se. The plan's Item 3 needs the guard added before implementation. +- **Item 3 Proxy Date/Map/Set/class-instance wrapping** — same. + +**Item ordering sanity:** +- Plan proposes landing 1, 2, 4 together. This is mechanical and low-risk — reasonable. +- Plan proposes Items 3, 5 after (Proxy wrapper + docs). If Item 3's two flaws are fixed in the implementation PR (safety-mode gate + plain-object-only wrap), this is fine. Otherwise Item 3 ships with regressions. +- Item 6 before Item 7 — correct, because the bench baseline rebuild matters for Item 7's profile. +- Item 8 follow-up — reasonable given low impact. + +**Scope:** +- Plan explicitly excludes `Signal.computed` / `Signal.derive` rewrites (line 227). Sound. +- No speculative micro-optimization (line 226). Disciplined. + +**Verdict on Q8**: One missing item (doc migration), no wrong items. + +## Summary + +Prioritized issues (all in Item 3 and Item 6): + +1. **Item 3: add `this.safety === 'freeze'` guard to the proxy wrapper.** Without it, `safety: 'reference'` and `safety: 'none'` signals get wrapped in mutation-blocking proxies in dev, breaking their documented semantics. One-line fix: + ```js + if (config.mode === 'off' || this.safety !== 'freeze' || val === null || typeof val !== 'object') { + return val; + } + ``` + +2. **Item 3: add `isPlainObject(val) || isArray(val)` guard to the proxy wrapper.** Date/Map/Set/class-instance signals aren't frozen by `deepFreeze` (per the `isPlainObject` gate at `cloning.js:31`) but the proposed proxy wraps them anyway. Method calls (including read-only ones like `getTime()`, `.has(k)`, `.get(k)`) throw incompatible-receiver TypeErrors in dev. Gate the wrap to match what's actually frozen: + ```js + if (!isPlainObject(val) && !isArray(val)) { return val; } + ``` + +3. **Item 6: include documentation sites in the migration audit.** The plan's list misses: + - `docs/src/pages/docs/api/reactivity/signal.mdx` (5 live references) + - `docs/src/pages/docs/guides/reactivity/signal-options.mdx` (2 references) + - `ai/skills/authoring/reactive-state.md`, `ai/skills/authoring/component-state.md`, `ai/skills/contributing/internals.md` + - `ai/workspace/artifacts/skills/sui-value-propositions.md` + + These are served to users (docs) or agents (skills) and will mislead after the shim is removed. + +Items 1, 2, 4, 5, 7, 8 check out against source. The preset tradeoff analysis and the "why not proxy-default for 1.0" reasoning are well-grounded. The `allowClone` autoresearch count is off by 2× (6 uses across 3 files, not 3 sites) but these files are in `ai/workspace/autoresearch/` — the plan already flags them as "check if still relevant or can be deleted." diff --git a/ai/workspace/signal-safety-v2-plan-review.md b/ai/workspace/signal-safety-v2-plan-review.md new file mode 100644 index 000000000..e58e8c053 --- /dev/null +++ b/ai/workspace/signal-safety-v2-plan-review.md @@ -0,0 +1,148 @@ +## Task: Review the signal-safety-v2 finalization plan for technical correctness + +Read this brief, then read the plan at `ai/workspace/plans/signal-safety-v2.md`, then read the source files listed below. Evaluate the plan's technical claims against the actual code. + +**Important framing**: "No changes needed" is a valid and welcome verdict. The plan is the output of a deep pairing session and may already be correct. Your job is to **steelman** the plan — assume reasonable authors, look for specific technical errors you can substantiate from the source, and flag them with evidence. Do not invent issues or propose stylistic rewrites. If a claim in the plan checks out against the code, say so and move on. + +--- + +### Architecture Overview + +`packages/reactivity` provides a Signal primitive loosely inspired by Meteor Tracker. A `Signal` wraps a value and registers dependencies with active `Reaction`s when read. Reactions re-run when their dependencies change, via a microtask-scheduled flush queue. + +A `safety` preset on each Signal governs how the Signal treats its value: + +- `'freeze'` — `deepFreeze` the value on `set`, return raw frozen value on `get`, dedupe via `isEqual`. Mutations through `get()` on frozen values trigger a native TypeError at the mutation site. +- `'reference'` — no protection; return the raw value on `get`, dedupe via `isEqual`. +- `'none'` — no protection, no dedupe (every `set` notifies). + +There is also a backward-compatibility shim where `{ allowClone: false }` maps to `safety: 'reference'` (to keep a tachometer benchmark fair across a PR/baseline split). + +The plan proposes flipping the default from `'reference'` (currently in the working tree as an unstaged change) back to `'freeze'` for 1.0, plus six other changes detailed in the plan document. + +### Key Source Mechanisms + +**`Signal.prototype.get` / setter / `protect`** — Signal's read returns `this.currentValue` (no clone). Setter calls `this.protect(newValue)` before assigning. `protect` gates on `safety === 'freeze'` to call `deepFreeze`; otherwise returns the raw value. The equality function is `isEqual` by default, or `() => false` under `'none'`. + +**`deepFreeze` in `packages/utils/src/cloning.js`** — Deep freezes plain objects and arrays recursively. Non-plain-object types (Date, Map, Set, RegExp, DOM nodes, class instances, Signals) are skipped per an `isPlainObject` gate. The source comment documents this as intentional to preserve internal-slot behavior of non-plain types. + +**`isEqual` in `packages/utils/src/equality.js`** — Identity short-circuits at the top (`if (a === b) return true`). Prototype comparison happens before deep walk. Specialized paths for Array, Map, Set, Date, RegExp, TypedArray, valueOf/toString, and plain-object key iteration. + +**`clone` in `packages/utils/src/cloning.js`** — Recursive clone with type-specific branches. Class instances are cloned as plain objects (prototype stripped) unless `{ preserveNonCloneable: true }` is passed. `Signal.defaultClone = clone` — without `preserveNonCloneable`. + +**`Signal.prototype.notify`** — Calls `this.setContext()` and `this.setTrace()` before `this.dependency.changed(this.context)`. `setContext` and `setTrace` have internal `config.mode !== 'off'` early-return guards. `Reaction.run` has a similar pattern around `addContext`. + +**`Signal.prototype.mutate`** — Under freeze, `fn` must return a new value (in-place mutation throws from the frozen value, not from mutate). Under reference/none, clones `currentValue` into `before`, runs `fn(currentValue)` which may mutate in place, then compares `before` vs `currentValue` via `equalityFunction` to decide whether to notify. + +**Mutation helpers** (`push`, `unshift`, `splice`, `setIndex`, `removeIndex`, `setArrayProperty`, `setProperty`) each have an explicit `safety === 'freeze'` branch that rebuilds the array/object vs an in-place branch for other modes. All call `notify()` directly, bypassing `mutate`. + +--- + +### Plan Summary (your target for evaluation) + +The plan has 8 numbered items: + +1. Flip default from `'reference'` back to `'freeze'` in `helpers.js`. +2. Dev-mode post-set reference check — console.warn when `set()` receives the same reference as `currentValue`. +3. Dev-mode Proxy wrapper on `.get()` returns to replace cryptic native freeze TypeErrors with SUI-authored errors. WeakMap-cached, top-level only (no recursive wrapping). +4. Hoist `config.mode !== 'off'` guard from inside `setContext`/`setTrace` up to `notify()` and `run()` call sites. +5. Document the third-party-reference escape hatch in the README with a user-facing heuristic. +6. Remove the `allowClone` backward-compat shim and rebuild the tachometer baseline. +7. Profile the residual ~+4% `remove-first` regression (known from a prior session; not diagnosed yet). +8. Latent follow-up: `Signal.defaultClone` doesn't pass `{ preserveNonCloneable: true }`, so the `before` snapshot in reference-mode `mutate()` strips class-instance prototypes. + +Plus a separate R&D branch for a proxy-default prototype with success criteria. + +--- + +### Questions — Evaluate Independently + +**Question 1 — Item 3 Proxy wrapper correctness.** Trace through each mutation shape and verify whether the plan's claimed behavior holds under the proposed implementation: + +- `signal.get().push(x)` on a signal holding an array +- `signal.get().prop = x` on a signal holding a plain object +- `signal.get().nested.prop = x` (access through the proxy to a nested frozen value) +- `signal.get().set(k, v)` on a signal holding a `Map` +- `signal.get().getTime()` on a signal holding a `Date` (read-only method — should not throw) +- `signal.get()` consumed by `JSON.stringify` or `structuredClone` + +The plan claims "top-level only, WeakMap-cached, nested access falls through to the raw frozen value which throws native TypeError." Is that accurate? Are there Proxy invariants or spec corner cases that would break this (e.g., `Object.isFrozen(proxy)` must match `Object.isFrozen(target)` under certain conditions)? + +**Question 2 — Claims about deepFreeze, isEqual, clone.** The plan makes assertions about: + +- `deepFreeze` skipping Map/Set/Date/class instances via `isPlainObject` +- `isEqual` having an identity short-circuit at the top +- `clone` not preserving class-instance prototypes by default +- `isEqual`'s `getProto` comparison in the context of the Item 8 bug description + +Verify each against the source code in `packages/utils/src/cloning.js` and `equality.js`. Are any of these claims wrong or imprecise? + +**Question 3 — Item 2 dev-mode check completeness.** The plan shows a trace validation for three cases (get-mutate-set, build-new, re-set-deep-equal). Are there additional cases the check would fire on incorrectly, or cases it should fire on but wouldn't? Consider primitives (numbers, strings), `null`/`undefined` transitions, frozen-object re-sets. + +**Question 4 — Item 4 hot-path guard hoist.** Reading `notify()`, `setContext()`, `setTrace()`, `Reaction.run()`, and `addContext()`: does the proposed hoist preserve current behavior? Specifically, does `this.context` remain accessible to `dependency.changed(this.context)` under mode 'off' after hoisting? Trace the code paths. + +**Question 5 — Item 6 callsite audit.** Grep for `allowClone` across the repo. Does the plan's list of migration sites cover all of them? Are any of the listed sites actually obsolete (scratch files, generated artifacts) rather than live code? + +**Question 6 — Item 8 bug description.** Read the `mutate` implementation in `signal.js` and trace what happens under reference mode when the signal holds a class instance: + +```js +const sig = new Signal(new MyClass({ foo: 1 })); +sig.mutate(v => { v.foo = 2; }); +``` + +Does the plan's description of the behavior match? What does `before` look like? Does `isEqual(before, currentValue)` return true or false? Does `notify()` fire? + +**Question 7 — Interactions between items.** Are there dependencies or conflicts between items that the plan doesn't address? For example: +- Does the Item 3 Proxy wrapper interact with Item 2's dev-mode check (both run under `config.mode !== 'off'`)? +- Does Item 4's hoist affect Item 7's profiling methodology (if the `notify` call shape changes, prior-session profile data may not be comparable)? +- Does Item 1's default flip invalidate Item 7's baseline (the residual regression was measured against the `'reference'` experiment)? + +**Question 8 — Missing items or wrong items.** Is there something in the Signal/Reactivity code that needs addressing but isn't in the plan? Or is there an item in the plan that shouldn't be done (e.g., too speculative, wrong priority, better deferred)? + +--- + +### Source Files to Read + +Required: +- `ai/workspace/plans/signal-safety-v2.md` (the plan itself) +- `packages/reactivity/src/signal.js` +- `packages/reactivity/src/helpers.js` +- `packages/reactivity/src/reaction.js` +- `packages/reactivity/src/dependency.js` +- `packages/reactivity/src/scheduler.js` +- `packages/utils/src/cloning.js` +- `packages/utils/src/equality.js` +- `packages/utils/src/types.js` (for `isPlainObject`, `isClassInstance`) + +As needed for specific questions: +- For Q5 (callsite audit): grep for `allowClone` and `safety:` across `packages/`, `src/`, `docs/src/`, `tools/`. +- For Q1 (Proxy wrapper): MDN / ECMAScript spec references for Proxy invariants are out of scope for this task — reason from first principles using the Proxy trap list. + +Do NOT read: +- `ai/workspace/signal-safety-v2-debrief.md` — contains solution momentum from prior sessions that's not load-bearing for this review. +- Git history / diffs — evaluate the plan against current source state only. + +--- + +### Deliverable + +Write your evaluation to `ai/workspace/signal-safety-v2-plan-review-report.md`. Structure: + +``` +## Verdict +[One of: Plan is correct as written / Plan has N specific issues / Plan has structural problem X] + +## Per-question findings +### Q1 — Item 3 Proxy wrapper correctness +[Your analysis with source references] + +### Q2 — Claims about utils +[...] + +[etc.] + +## Summary +[If issues: prioritized list. If no issues: brief confirmation of what you verified.] +``` + +Include file:line references for every claim. If you find an issue, quote the relevant source and explain the specific divergence from the plan's claim. diff --git a/docs/src/examples/reactivity/advanced/reactive-notify/index.js b/docs/src/examples/reactivity/advanced/reactive-notify/index.js index 077e5785a..416282fdf 100644 --- a/docs/src/examples/reactivity/advanced/reactive-notify/index.js +++ b/docs/src/examples/reactivity/advanced/reactive-notify/index.js @@ -1,6 +1,6 @@ import { Reaction, Signal } from '@semantic-ui/reactivity'; -const data = new Signal({ count: 0 }, { allowClone: false }); +const data = new Signal({ count: 0 }, { safety: 'reference' }); Reaction.create(() => { console.log('Count:', data.get().count); diff --git a/docs/src/examples/reactivity/variables/reactive-clone/index.js b/docs/src/examples/reactivity/variables/reactive-clone/index.js index 7abbacb35..d6c9b539c 100644 --- a/docs/src/examples/reactivity/variables/reactive-clone/index.js +++ b/docs/src/examples/reactivity/variables/reactive-clone/index.js @@ -1,29 +1,34 @@ /* - By default accidental mutations are protected because set/get are cloned - using allowClone: false will remove this protection but avoid cloning overhead + Signals default to safety: 'freeze' — the stored value is deep-frozen, + so accidental in-place mutation throws at the call site. + Pass safety: 'reference' to opt a signal out of freezing (e.g. when it + holds third-party objects that mutate their own state). */ import { Reaction, Signal } from '@semantic-ui/reactivity'; -const safeItems = new Signal(['apple', 'banana']); -const unsafeItems = new Signal(['apple', 'banana'], { allowClone: false }); +const safe = new Signal(['apple', 'banana']); +const unsafe = new Signal(['apple', 'banana'], { safety: 'reference' }); Reaction.create(() => { - console.log('Safe items:', safeItems.get()); + console.log('Safe items:', safe.get()); }); Reaction.create(() => { - console.log('Unsafe items:', unsafeItems.get()); + console.log('Unsafe items:', unsafe.get()); }); -console.log('--- Accidental mutation ---'); - -// Get references and try to mutate them -const safeRef = safeItems.get(); -const unsafeRef = unsafeItems.get(); +// Correct way to add an item under either mode: use the mutation helper +safe.push('orange'); +unsafe.push('orange'); +Reaction.flush(); -// somewhere else in code accidentally mutate the copy -safeRef.push('cherry'); -unsafeRef.push('cherry'); +// Direct mutation on the reference: throws under freeze, silent no-op under reference +try { + safe.get().push('cherry'); +} +catch (e) { + console.log('Caught:', e.message); +} -safeItems.push('orange'); -unsafeItems.push('orange'); -Reaction.flush(); +// Under reference, this mutates the stored array in place but does NOT notify subscribers +unsafe.get().push('cherry'); +console.log('Unsafe after silent mutation:', unsafe.peek()); diff --git a/docs/src/examples/utils/functions/utils-noop/index.js b/docs/src/examples/utils/functions/utils-noop/index.js index 4411f2a62..8c0b17ce1 100644 --- a/docs/src/examples/utils/functions/utils-noop/index.js +++ b/docs/src/examples/utils/functions/utils-noop/index.js @@ -1,15 +1,14 @@ import { noop } from '@semantic-ui/utils'; -const value = 'hello'; -const result = noop(value); -console.log(result); +// noop swallows arguments and returns undefined +console.log(noop()); // undefined +console.log(noop(1, 2, 3)); // undefined -// common use case - default callback +// common use case - default callback that is safe to invoke function processData(data, callback = noop) { const processed = data.toUpperCase(); callback(processed); return processed; } -const data = processData('test'); -console.log(data); +console.log(processData('test')); // 'TEST' diff --git a/docs/src/pages/docs/api/reactivity/signal.mdx b/docs/src/pages/docs/api/reactivity/signal.mdx index 254ea1eb4..e7f84a4bc 100755 --- a/docs/src/pages/docs/api/reactivity/signal.mdx +++ b/docs/src/pages/docs/api/reactivity/signal.mdx @@ -30,9 +30,9 @@ new Signal(initialValue, options); | Name | Type | Default | Description | |------|------|---------|-------------| +| safety | `'freeze'` \| `'reference'` \| `'none'` | `'freeze'` | Value-protection preset. See [Signal Options](/docs/guides/reactivity/signal-options) for details. | | equalityFunction | Function | isEqual | Custom function to determine if the value has changed | -| allowClone | boolean | true | Whether to clone the value when getting/setting | -| cloneFunction | Function | clone | Custom function to clone the value | +| cloneFunction | Function | clone | Custom function used by `signal.clone()` | ### Usage @@ -54,28 +54,18 @@ const person = new Signal({ name: 'John', age: 30 }, { }); ``` -#### Disabling Cloning for Custom Classes +#### Holding Borrowed Data -To avoid mutating the source object naively, by default values are cloned when set. However some objects cannot be naively cloned like custom classes. +The default `safety: 'freeze'` deep-freezes object and array values on `set` to catch accidental in-place mutation. For signals holding objects you don't own — anything returned from a library, fetched from an API, or passed through a callback — opt out with `safety: 'reference'` so the library's own reference isn't poisoned. ```javascript import { Signal } from '@semantic-ui/reactivity'; -class CustomClass { - constructor(value) { - this.value = value; - } -} - -const customInstance = new Signal(new CustomClass(42), { - allowClone: false, - equalityFunction: (a, b) => a.value === b.value -}); - -// The CustomClass instance won't be cloned when accessed -console.log(customInstance.get().value); // Output: 42 +const searchResults = new Signal(pagefindResults, { safety: 'reference' }); ``` +See the [Signals and Foreign References](/docs/guides/reactivity/signals#signals-and-foreign-references) section of the signals guide for the full heuristic. + ## `Get` Returns the current value of the reactive variable. @@ -212,7 +202,7 @@ someValue.notify(); ```javascript import { Reaction, Signal } from '@semantic-ui/reactivity'; -const canvas = new Signal(document.createElement('canvas'), { allowClone: false }); +const canvas = new Signal(document.createElement('canvas'), { safety: 'reference' }); Reaction.create(() => { const el = canvas.get(); @@ -298,7 +288,7 @@ console.log(counter.get()); // Output: undefined ## `Mutate` -Safely mutates the current value using a mutation function. This method ensures reactivity is triggered even when `allowClone` is false or when using custom equality functions. +Safely mutates the current value using a mutation function. Under `safety: 'freeze'` the mutation function must return a new value (in-place mutation throws). Under `safety: 'reference'` or `'none'` the function may mutate in place and reactivity still fires. ### Syntax ```javascript @@ -317,24 +307,28 @@ The return value of the mutation function, if any. ### Usage -#### In-place Mutation +#### Returning a New Value (works under all safety modes) ```javascript import { Signal } from '@semantic-ui/reactivity'; const items = new Signal(['apple', 'banana']); -items.mutate(arr => { - arr.push('orange'); // Mutate in place -}); +items.mutate(arr => [...arr, 'orange']); console.log(items.get()); // ['apple', 'banana', 'orange'] + +const count = new Signal(5); +count.mutate(val => val * 2); +console.log(count.get()); // 10 ``` -#### Returning a New Value +#### In-place Mutation (only under safety: 'reference' or 'none') ```javascript import { Signal } from '@semantic-ui/reactivity'; -const count = new Signal(5); -count.mutate(val => val * 2); // Return new value -console.log(count.get()); // 10 +const items = new Signal(['apple', 'banana'], { safety: 'reference' }); +items.mutate(arr => { + arr.push('orange'); // Mutate in place — allowed because the value isn't frozen +}); +console.log(items.get()); // ['apple', 'banana', 'orange'] ``` #### With Custom Classes @@ -350,12 +344,13 @@ class Counter { } } -const counter = new Signal(new Counter(0), { - allowClone: false, - equalityFunction: (a, b) => a.value === b.value +// Class instances aren't frozen by default (deepFreeze skips them), +// but set safety: 'reference' explicitly when you plan to mutate in place +const counter = new Signal(new Counter(0), { + safety: 'reference', + equalityFunction: (a, b) => a.value === b.value, }); -// Safe mutation that triggers reactivity counter.mutate(c => c.increment()); console.log(counter.get().value); // 1 ``` diff --git a/docs/src/pages/docs/guides/reactivity/signal-options.mdx b/docs/src/pages/docs/guides/reactivity/signal-options.mdx index f36619254..47608ded0 100644 --- a/docs/src/pages/docs/guides/reactivity/signal-options.mdx +++ b/docs/src/pages/docs/guides/reactivity/signal-options.mdx @@ -44,60 +44,59 @@ const customEquality = (a, b) => { const customVar = new Signal(initialValue, { equalityFunction: customEquality }); ``` -## Value Cloning +## Safety Presets -By default, Signals automatically clone object and array values during [`get`](/docs/api/reactivity/signal#get) and [`set`](/docs/api/reactivity/signal#set) operations. +Signals have three safety presets controlling how the stored value is protected against accidental mutation. Set the preset via the `safety` option. -A very common issue when using naive Signals implementations is that it can be very easy to accidentally update an underlying signal value when modifying its value without using `set()` or `value`. +| Preset | On `set` | On `.get().x = y` | Dedupe | Use case | +|---|---|---|---|---| +| `freeze` (default) | deep-freeze plain objects and arrays | throws `TypeError` | `isEqual` | state your code owns end-to-end | +| `reference` | store raw | silent (no reactivity) | `isEqual` | third-party objects, perf-critical paths | +| `none` | store raw | silent (no reactivity) | never | event-stream semantics — every `set` notifies | -```javascript -const countObj = new Signal({ count: 0 }); +### `safety: 'freeze'` — the default -// by default this will not update the current count unless set() is called -const newObj = countObj.get(); -newObj.count = 1; -``` +The default deep-freezes object and array values when you call `set()`. Accidental in-place mutation throws at the call site instead of silently dropping the update. -Cloning prevents accidental mutation of the Signal's internal state and ensures reliable [equality checks](#equality-comparison). +```javascript +const count = new Signal({ n: 0 }); -Disabling cloning (using the [`allowClone`](/docs/api/reactivity/signal#options) option) will reduce the overhead of `set` and `get` calls but will potentially cause unexpected behaviors when modifying arrays and objects. -```javascript title="Accidental Mutations" -const countObj = new Signal({ count: 0 }, { allowClone: false }); +count.get().n = 1; // TypeError — "Signal value is frozen — cannot set property `n`" -// this will not trigger reactivity, but the value will change in the next flush -// this is because the underlying signal was accidentally mutated -const newObj = countObj.get(); -newObj.count = 1; +// Correct ways to update: +count.set({ n: 1 }); // replace the whole value +count.mutate(prev => ({ n: prev.n + 1 })); // return a new value +count.setProperty('n', 1); // mutation helper — rebuilds immutably under freeze ``` -### Custom Cloning Function +Deep-freeze only walks arrays and plain objects. `Date`, `Map`, `Set`, `RegExp`, DOM nodes, and class instances are stored by reference — their own mutation semantics are preserved. -Similar to the equality check, the cloning function itself can be customized by providing a [`cloneFunction`](/docs/api/reactivity/signal#options) in the constructor options. +### `safety: 'reference'` — opt-out for borrowed data -```javascript -// Simple JSON clone -const customClone = (value) => { - return JSON.parse(JSON.stringify(value)); -}; +Use `reference` when the signal holds objects you didn't construct yourself. Freezing them would poison the lender's internal references; see [Signals and Foreign References](/docs/guides/reactivity/signals#signals-and-foreign-references) for the full heuristic. -// Signal using custom clone function -const customCloneSignal = new Signal({ data: 1 }, { cloneFunction: customClone }); +```javascript +const searchResults = new Signal([], { safety: 'reference' }); ``` -### Uncloneable Content +Direct mutation on `.get()` values fails silently under `reference` — the helpers (`push`, `splice`, `setProperty`) remain the safe update path. -Some values do not have reliable ways to clone. For instance, there is no universal way to clone a custom class. +### `safety: 'none'` — event-stream semantics -**Class instances are not cloned** by default, regardless of the `allowClone` setting. Signals always store and return direct references to class instances. +Use `none` when every `set` should notify subscribers, even if the value is deeply equal to the previous one. Suitable for notification channels where the payload's shape repeats. ```javascript -class MyData { - constructor(value) { this.value = value; } -} +const pulse = new Signal(null, { safety: 'none' }); + +pulse.set({ type: 'heartbeat' }); +pulse.set({ type: 'heartbeat' }); // still notifies, even though isEqual would say equal +``` -const instanceSignal = new Signal(new MyData(10)); +## Custom Clone Function -const instance1 = instanceSignal.get(); -const instance2 = instanceSignal.get(); -console.log(instance1 === instance2); +The default `cloneFunction` is used by `signal.clone()` to produce a deep-mutable copy on demand. Override it if you need non-default clone semantics. + +```javascript +const jsonClone = (value) => JSON.parse(JSON.stringify(value)); +const mySignal = new Signal({ data: 1 }, { cloneFunction: jsonClone }); ``` diff --git a/docs/src/pages/docs/guides/reactivity/signals.mdx b/docs/src/pages/docs/guides/reactivity/signals.mdx index 897da1f79..f76a4caf5 100644 --- a/docs/src/pages/docs/guides/reactivity/signals.mdx +++ b/docs/src/pages/docs/guides/reactivity/signals.mdx @@ -67,6 +67,44 @@ counter.increment(); // Equivalent to counter.set(counter.peek() + 1) console.log(counter.get()); // Output: 2 ``` +## Signals and Foreign References + +By default, Signals deep-freeze object and array values on `set()`. This catches the most common reactivity bug — mutating a value in place without notifying subscribers — by throwing a `TypeError` at the mutation site instead of silently dropping the update. + +Deep-freezing has one important caveat: if the object you store is *also held internally by a library*, freezing it will break that library the next time it mutates its own reference. + +> **When you need `{ safety: 'reference' }`**: if you're storing an object in a signal that you did not construct yourself — anything returned from a library, fetched from an API, or passed through a callback — default to `safety: 'reference'`. Freeze is the right default for state your own code owns end-to-end. For borrowed data, `reference` avoids poisoning the lender's internal references. + +### Worked Example: Search Index + +Pagefind returns result objects and continues to use them internally — subsequent `.data()` calls on each result mutate pagefind's own cached state. Storing the results under the default freeze mode freezes pagefind's internal objects and later crashes its loader: + +```javascript +const defaultState = { + // ❌ default freeze — pagefind's internal mutation of the stored objects will throw + rawResults: [], +}; +``` + +Opt this specific signal out of freeze: + +```javascript +const defaultState = { + // ✓ signal holds third-party-owned data; don't freeze + rawResults: { value: [], options: { safety: 'reference' } }, +}; +``` + +The rest of your component state keeps the default freeze protection — only the signal carrying borrowed data opts out. + +### Ad-hoc Construction + +For signals created outside a `defaultState` declaration, pass the preset as the second argument: + +```javascript +const results = new Signal(pagefindData, { safety: 'reference' }); +``` + ## Creating Derived Signals Signals can be transformed and combined to create new reactive values: diff --git a/internal-packages/esbuild-resolve-bare-imports/test/cdn-urls.test.js b/internal-packages/esbuild-resolve-bare-imports/test/cdn-urls.test.js index 4bb2514c3..ea6d302c5 100644 --- a/internal-packages/esbuild-resolve-bare-imports/test/cdn-urls.test.js +++ b/internal-packages/esbuild-resolve-bare-imports/test/cdn-urls.test.js @@ -6,7 +6,7 @@ const ROOT = resolve(import.meta.dirname, '../../..'); const CDN_ROOT = 'https://cdn.semantic-ui.com'; const SUI_PACKAGES = readdirSync(join(ROOT, 'packages'), { withFileTypes: true }) - .filter(d => d.isDirectory()) + .filter(d => d.isDirectory() && !d.name.startsWith('.')) .map(d => d.name); // Collect declared subpath exports (e.g. "./template") from each package diff --git a/packages/component/src/define-component.js b/packages/component/src/define-component.js index fd7dde5b5..fce0d4e0a 100644 --- a/packages/component/src/define-component.js +++ b/packages/component/src/define-component.js @@ -2,7 +2,7 @@ import { getEngine } from '@semantic-ui/renderer'; import { TemplateCompiler } from '@semantic-ui/compiler'; // direct import avoids circular chunk dependency between component ↔ templating import { Template } from '@semantic-ui/templating/template'; -import { adoptStylesheet, each, fatal, isClient, kebabToCamel, noop } from '@semantic-ui/utils'; +import { adoptStylesheet, each, fatal, identity, isClient, kebabToCamel, noop } from '@semantic-ui/utils'; import { getProperties } from './component-helpers.js'; import { registerComponent } from './component-registry.js'; @@ -16,7 +16,7 @@ export const defineComponent = ({ delegatesFocus = false, templateName = kebabToCamel(tagName), - createComponent: createComponentFn = noop, + createComponent: createComponentFn = identity, events = {}, keys = {}, diff --git a/packages/component/src/helpers.js b/packages/component/src/helpers.js index ee4816575..8d3fc5128 100644 --- a/packages/component/src/helpers.js +++ b/packages/component/src/helpers.js @@ -1,18 +1,17 @@ -import { - setStackCapture as setReactivityStackCapture, - setTracing as setReactivityTracing, -} from '@semantic-ui/reactivity'; +import { Signal } from '@semantic-ui/reactivity'; import { setRecovery as setRendererRecovery, setTracing as setRendererTracing } from '@semantic-ui/renderer'; // Cheap path — context naming + structured block error log. Default on // in dev for reactivity (renderer tracing stays off; the always-on // breadcrumb covers visibility there). export const setTracing = (enabled) => { - setReactivityTracing(enabled); + Signal.tracing = enabled; setRendererTracing(enabled); }; // Expensive path — Error.captureStackTrace per Signal.notify. Opt-in. -export const setStackCapture = setReactivityStackCapture; +export const setStackCapture = (enabled) => { + Signal.stackCapture = enabled; +}; export const setRecovery = setRendererRecovery; diff --git a/packages/query/src/behavior.js b/packages/query/src/behavior.js index a4d526f71..428f2d01c 100644 --- a/packages/query/src/behavior.js +++ b/packages/query/src/behavior.js @@ -4,6 +4,7 @@ import { clone, each, extend, + identity, isClassInstance, isFunction, isPlainObject, @@ -26,7 +27,7 @@ export class Behavior { namespace = name, // returns behavior instance - createBehavior = noop, + createBehavior = identity, // event object events = {}, @@ -57,7 +58,7 @@ export class Behavior { onDestroyed = noop, // custom invocation fallback - customInvocation = noop, + customInvocation = identity, // element index information elementIndex = 0, diff --git a/packages/reactivity/src/dependency.js b/packages/reactivity/src/dependency.js index 750339779..720bca067 100755 --- a/packages/reactivity/src/dependency.js +++ b/packages/reactivity/src/dependency.js @@ -1,10 +1,10 @@ -import { captureStack, isTracing } from './helpers.js'; +import { captureStack, config } from './helpers.js'; import { Scheduler } from './scheduler.js'; export class Dependency { constructor(...metadata) { this.subscribers = new Set(); - if (isTracing()) { + if (config.mode !== 'off') { this.setContext(metadata); } } @@ -16,17 +16,8 @@ export class Dependency { } } - // Cheap context naming on tracing; stack capture only on stack mode. - setContext(context) { - if (!isTracing()) { - return; - } - this.context = context || {}; - captureStack(this, this.setContext); - } - changed(context) { - if (isTracing()) { + if (config.mode !== 'off') { if (context) { this.context = context; } @@ -40,13 +31,23 @@ export class Dependency { } } - // called after flush cleanUp(reaction) { this.subscribers.delete(reaction); } - // identical for now but called from stop() unsubscribe(reaction) { this.subscribers.delete(reaction); } + + /*------------------- + Tracing + --------------------*/ + + setContext(context) { + if (config.mode === 'off') { + return; + } + this.context = context || {}; + captureStack(this, this.setContext); + } } diff --git a/packages/reactivity/src/helpers.js b/packages/reactivity/src/helpers.js index b366c3515..c20a00e6a 100644 --- a/packages/reactivity/src/helpers.js +++ b/packages/reactivity/src/helpers.js @@ -1,41 +1,21 @@ -// Tracing mode controls debug metadata attached to Signals, Reactions, and -// Dependencies. One field instead of two booleans — stack implies context, -// and the impossible "stack without context" state is unrepresentable. -// 'off' — no context, no allocation on notify -// 'context' — cheap: attach { firstRun, value, ... } bags for naming -// 'stack' — expensive: Error.captureStackTrace per notify, on top of context -let mode = 'off'; +export const config = { + // debug metadata attached during notify + // 'off' — zero cost; no allocation + // 'context' — attach { firstRun, value, ... } bags for naming + // 'stack' — Error.captureStackTrace per notify, on top of context + mode: 'off', -export const setTracing = (enabled) => { - if (enabled) { - if (mode === 'off') { mode = 'context'; } - } - else { - mode = 'off'; - } + // default value protection on set for new signals + // 'reference' — no protection; dedupe via isEqual + // 'freeze' — deepFreeze on set; downstream mutations throw + // 'none' — no protection, no dedupe (event-stream semantics) + safety: 'freeze', }; -export const setStackCapture = (enabled) => { - if (enabled) { - mode = 'stack'; - } - else if (mode === 'stack') { - mode = 'context'; - } -}; - -export const isTracing = () => mode !== 'off'; -export const isStackCapture = () => mode === 'stack'; - -// Capture a stack trace into target.context. Passes `caller` to -// Error.captureStackTrace so the framework frame is trimmed from the trace. +// `caller` is passed to Error.captureStackTrace so the framework frame is trimmed from the trace export const captureStack = (target, caller) => { - if (mode !== 'stack') { - return; - } - if (!target.context) { - target.context = {}; - } + if (config.mode !== 'stack') { return; } + if (!target.context) { target.context = {}; } if (Error.captureStackTrace) { Error.captureStackTrace(target.context, caller); } @@ -43,3 +23,6 @@ export const captureStack = (target, caller) => { target.context.stack = new Error().stack; } }; + +// solves 'instanceof Signal' checks if across realms or package duplication +export const signalTag = Symbol.for('semantic-ui/Signal'); diff --git a/packages/reactivity/src/index.js b/packages/reactivity/src/index.js index dc2d6c37d..fec3595a4 100755 --- a/packages/reactivity/src/index.js +++ b/packages/reactivity/src/index.js @@ -1,5 +1,4 @@ export { Dependency } from './dependency.js'; -export { isStackCapture, isTracing, setStackCapture, setTracing } from './helpers.js'; export { Reaction } from './reaction.js'; export { Scheduler } from './scheduler.js'; export { Signal } from './signal.js'; diff --git a/packages/reactivity/src/reaction.js b/packages/reactivity/src/reaction.js index b5d495ab5..a2f3939a3 100755 --- a/packages/reactivity/src/reaction.js +++ b/packages/reactivity/src/reaction.js @@ -1,6 +1,6 @@ -import { clone, isEqual } from '@semantic-ui/utils'; +import { isEqual } from '@semantic-ui/utils'; import { Dependency } from './dependency.js'; -import { captureStack, isStackCapture, isTracing, setStackCapture, setTracing } from './helpers.js'; +import { captureStack, config } from './helpers.js'; import { Scheduler } from './scheduler.js'; export class Reaction { @@ -17,31 +17,14 @@ export class Reaction { this.dependencies = new Set(); this.firstRun = true; this.active = true; - if (context && isTracing()) { + if (context && config.mode !== 'off') { this.setContext(context); } this.boundRun = this.run.bind(this); } - setContext(additionalContext = {}) { - if (!isTracing()) { - return; - } - const defaultContext = { - firstRun: this.firstRun, - }; - this.context = { - ...defaultContext, - ...additionalContext, - }; - } - - setTrace() { - captureStack(this, this.setTrace); - } - addContext(additionalContext = {}) { - if (!isTracing()) { + if (config.mode === 'off') { return; } if (!this.context) { @@ -53,11 +36,10 @@ export class Reaction { } run() { - // only run this reaction is marked as active if (!this.active) { return; } - if (isTracing()) { + if (config.mode !== 'off') { this.addContext({ firstRun: this.firstRun, }); @@ -77,15 +59,12 @@ export class Reaction { } invalidate(context) { - // Set this reaction as active and about to be run this.active = true; - // Pass through trace for debugging - if (context) { + if (context && config.mode !== 'off') { this.addContext(context); } - // Schedule this reaction to occur in the next flush Scheduler.scheduleReaction(this); } @@ -97,20 +76,18 @@ export class Reaction { this.dependencies.forEach(dep => dep.unsubscribe(this)); } - // Static proxies for developer experience static get current() { return Scheduler.current; } - // DX pass throughs + /*------------------- + Helpers + --------------------*/ + static flush = Scheduler.flush; static scheduleFlush = Scheduler.scheduleFlush; static afterFlush = Scheduler.afterFlush; static getSource = Scheduler.getSource; - static setTracing = setTracing; - static isTracing = isTracing; - static setStackCapture = setStackCapture; - static isStackCapture = isStackCapture; static nonreactive(func) { const previousReaction = Scheduler.current; @@ -140,4 +117,25 @@ export class Reaction { comp.run(); return newValue; } + + /*------------------- + Tracing + --------------------*/ + + setContext(additionalContext = {}) { + if (config.mode === 'off') { + return; + } + const defaultContext = { + firstRun: this.firstRun, + }; + this.context = { + ...defaultContext, + ...additionalContext, + }; + } + + setTrace() { + captureStack(this, this.setTrace); + } } diff --git a/packages/reactivity/src/scheduler.js b/packages/reactivity/src/scheduler.js index be64b8147..82a3ef6eb 100755 --- a/packages/reactivity/src/scheduler.js +++ b/packages/reactivity/src/scheduler.js @@ -1,25 +1,10 @@ import { microtask } from '@semantic-ui/utils'; -const flushTask = () => Scheduler.flush(); - export class Scheduler { static current = null; static pendingReactions = new Set(); static afterFlushCallbacks = []; static isFlushScheduled = false; - - static scheduleReaction(reaction) { - Scheduler.pendingReactions.add(reaction); - Scheduler.scheduleFlush(); - } - - static scheduleFlush() { - if (!Scheduler.isFlushScheduled) { - Scheduler.isFlushScheduled = true; - microtask(flushTask); - } - } - static maxFlushIterations = 100; static flush() { @@ -45,10 +30,30 @@ export class Scheduler { } } + /*------------------- + Helpers + --------------------*/ + static afterFlush(callback) { Scheduler.afterFlushCallbacks.push(callback); } + static scheduleReaction(reaction) { + Scheduler.pendingReactions.add(reaction); + Scheduler.scheduleFlush(); + } + + static scheduleFlush() { + if (!Scheduler.isFlushScheduled) { + Scheduler.isFlushScheduled = true; + microtask(Scheduler.flush); + } + } + + /*------------------- + Tracing + --------------------*/ + static getSource() { if (!Scheduler.current) { console.log('No reactive flush is currently occurring.'); diff --git a/packages/reactivity/src/signal.js b/packages/reactivity/src/signal.js index aa27ce0d9..155b461e7 100755 --- a/packages/reactivity/src/signal.js +++ b/packages/reactivity/src/signal.js @@ -1,273 +1,281 @@ import { clone, + deepFreeze, findIndex, isArray, - isClassInstance, - isDevelopment, isEqual, isNumber, isObject, + isPlainObject, unique, wrapFunction, } from '@semantic-ui/utils'; import { Dependency } from './dependency.js'; -import { captureStack, isStackCapture, isTracing, setStackCapture, setTracing } from './helpers.js'; +import { captureStack, config, signalTag } from './helpers.js'; import { Reaction } from './reaction.js'; -const IS_SIGNAL = Symbol.for('semantic-ui/Signal'); +const devProxyCache = new WeakMap(); + +const frozenTraps = { + set(_, prop) { + throw frozenError(prop); + }, + deleteProperty(_, prop) { + throw frozenError(prop); + }, + defineProperty(_, prop) { + throw frozenError(prop); + }, + setPrototypeOf() { + throw frozenError('[[Prototype]]'); + }, +}; + +function frozenError(prop) { + return new TypeError( + `Signal value is frozen — cannot set property \`${String(prop)}\`. ` + + `Use signal.set(newValue), a mutation helper (push, splice, setProperty), ` + + `or construct with { safety: 'reference' } if storing third-party data.`, + ); +} + +function devProxyFor(val) { + let proxy = devProxyCache.get(val); + if (!proxy) { devProxyCache.set(val, proxy = new Proxy(val, frozenTraps)); } + return proxy; +} export class Signal { - get [IS_SIGNAL]() { - return true; - } - static [Symbol.hasInstance](instance) { - return !!instance?.[IS_SIGNAL]; - } + constructor(initialValue, options = {}) { + const { context, safety, equalityFunction, cloneFunction } = options; - constructor(initialValue, { context, equalityFunction, allowClone = true, cloneFunction } = {}) { - // pass in some metadata for debugging this.dependency = new Dependency({ firstRun: true, value: initialValue, }); - // allow user to opt out of value cloning - this.allowClone = allowClone; + this.safety = safety ?? config.safety; - // allow custom equality function - this.equalityFunction = (equalityFunction) + this.equalityFunction = equalityFunction ? wrapFunction(equalityFunction) - : Signal.equalityFunction; + : (this.safety === 'none' ? Signal.noEquality : Signal.defaultEquality); - // allow custom clone function - this.clone = (cloneFunction) + this.cloneFunction = cloneFunction ? wrapFunction(cloneFunction) - : Signal.cloneFunction; + : Signal.defaultClone; - this.currentValue = this.maybeClone(initialValue); + this.currentValue = this.protect(initialValue); - // allow debugging context to be set this.setContext(context); } - // set debugging context for signal removing any present context - setContext(additionalContext = {}) { - if (!isTracing()) { - return; - } - const defaultContext = { - value: this.currentValue, - }; - this.context = { - ...defaultContext, - ...additionalContext, - }; + /*------------------- + Core + --------------------*/ + + protect(value) { + if (value === null || typeof value !== 'object') { return value; } + return this.safety === 'freeze' ? deepFreeze(value) : value; } - // add context to signal - addContext(additionalContext = {}) { - if (!isTracing()) { + get value() { + this.depend(); + const val = this.currentValue; + if (config.mode === 'off' || this.safety !== 'freeze') { return val; } + if (isArray(val) || isPlainObject(val)) { return devProxyFor(val); } + return val; + } + + set value(newValue) { + if (!this.equalityFunction(this.currentValue, newValue)) { + this.currentValue = this.protect(newValue); + this.notify(); return; } - if (!this.context) { - this.context = {}; - } - for (const key in additionalContext) { - this.context[key] = additionalContext[key]; + if (config.mode !== 'off' && isObject(newValue) && newValue === this.currentValue) { + console.warn( + 'Signal.set() called with the same reference as the current value. ' + + 'If you mutated in place, the reactive update is lost. ' + + 'Use signal.push/splice/setProperty, or return a new value from mutate(). ' + + 'If this was intentional, construct a new value and set that instead.', + ); } } - // Stack trace capture is gated separately because Error.captureStackTrace - // costs ~10-100× a context spread, paid per Signal.notify in tracing-on - // dev. Default off; opt in via setStackCapture(true). - setTrace() { - captureStack(this, this.setTrace); + get() { + return this.value; } - static equalityFunction = isEqual; - static cloneFunction = clone; - static setTracing = setTracing; - static isTracing = isTracing; - static setStackCapture = setStackCapture; - static isStackCapture = isStackCapture; + set(newValue) { + this.value = newValue; + } - get value() { - // Record this Signal as a dependency if inside a Reaction computation - this.depend(); - const value = this.currentValue; + peek() { + return this.currentValue; + } - // otherwise previous value would be modified if the returned value is mutated negating the equality - return (value !== null && typeof value == 'object') - ? this.maybeClone(value) - : value; + clone() { + this.depend(); + return this.cloneFunction(this.currentValue); } - canCloneValue(value) { - return (this.allowClone === true && !isClassInstance(value)); + subscribe(callback) { + return Reaction.create((comp) => { + callback(this.value, comp); + }); } - maybeClone(value) { - if (!this.canCloneValue(value)) { - return value; - } - if (isArray(value)) { - return value.map(value => this.maybeClone(value)); - } - return this.clone(value); + depend() { + this.dependency.depend(); } - set value(newValue) { - if (!this.equalityFunction(this.currentValue, newValue)) { - this.currentValue = this.maybeClone(newValue); - this.notify(); + notify() { + if (config.mode !== 'off') { + this.setContext(); + this.setTrace(); } + this.dependency.changed(this.context); } - get({ clone = true } = {}) { - if (!clone) { - this.depend(); - return this.currentValue; - } - return this.value; + hasDependents() { + return this.dependency.subscribers.size > 0; } - set(newValue) { - // equality check in setter - this.value = newValue; + clear() { + return this.set(undefined); } - subscribe(callback) { - return Reaction.create((comp) => { - callback(this.value, comp); + /*------------------- + Complex + --------------------*/ + + static computed(computeFn, options = {}) { + const computedSignal = new Signal(undefined, options); + const reaction = Reaction.create(() => { + computedSignal.set(computeFn()); }); + computedSignal._computedReaction = reaction; + return computedSignal; } - // derive a new signal from this signal's value derive(computeFn, options = {}) { const derivedSignal = new Signal(undefined, options); - - // check if signal has been garbage collected - // if it has we need to clean up reaction + // WeakRef lets the derived reaction self-stop when the source is GC'd const sourceRef = new WeakRef(this); - // Create reaction that updates the derived signal const reaction = Reaction.create(() => { const source = sourceRef.deref(); if (!source) { - reaction.stop(); // Auto-cleanup if source is gone + reaction.stop(); return; } const result = computeFn(source.get()); derivedSignal.set(result); }); - // Store reaction reference for potential cleanup derivedSignal._derivedReaction = reaction; return derivedSignal; } - // static method for computing from multiple signals - static computed(computeFn, options = {}) { - const computedSignal = new Signal(undefined, options); - - // Create reaction that updates the computed signal - // No WeakRef needed - computed signal and reaction have same lifecycle - const reaction = Reaction.create(() => { - const result = computeFn(); - computedSignal.set(result); - }); - - // Store reaction reference for potential cleanup - computedSignal._computedReaction = reaction; - - return computedSignal; - } - - depend() { - this.dependency.depend(); - } - - notify() { - // Each gate handles itself — setContext on isTracing, setTrace on - // isStackCapture. Hot path: both early-return when their flag is off. - this.setContext(); - this.setTrace(); - this.dependency.changed(this.context); - } - - hasDependents() { - return this.dependency.subscribers.size > 0; - } - - peek() { - return this.maybeClone(this.currentValue); - } - - clear() { - return this.set(undefined); - } - - // mutate the current value by a mutation function - mutate(mutationFn) { - // we use clone in all cases to detect for changes only - const beforeClone = this.clone(this.currentValue); - const result = mutationFn(this.currentValue); + /*------------------- + Mutation Helpers + --------------------*/ - if (result !== undefined) { - if (isDevelopment && result === this.currentValue) { - console.warn( - 'Signal.mutate: returning the same reference that was mutated in place will bypass change detection. Either mutate without returning, or return a new value.', - ); + mutate(fn) { + // freeze: fn must return a new value (in-place throws). + // reference/none: fn may mutate in place; dedupe via equalityFunction. + if (this.safety === 'freeze') { + const result = fn(this.currentValue); + if (result !== undefined) { + this.value = result; } - // if the mutation returned a value just set it - this.value = result; - } - else { - // if no value returned check if the value changed from side effects - // in this case we want to trigger reactivity - if (!this.equalityFunction(beforeClone, this.currentValue)) { + else { this.notify(); } + return; + } + const before = this.cloneFunction(this.currentValue); + const result = fn(this.currentValue); + if (result !== undefined) { + this.value = result; + } + else if (!this.equalityFunction(before, this.currentValue)) { + this.notify(); } } - // array helpers — these always change the value, skip clone+compare push(...args) { - this.currentValue.push(...args); + if (this.safety === 'freeze') { + this.currentValue = this.protect([...this.currentValue, ...args]); + } + else { + this.currentValue.push(...args); + } this.notify(); } + unshift(...args) { - this.currentValue.unshift(...args); + if (this.safety === 'freeze') { + this.currentValue = this.protect([...args, ...this.currentValue]); + } + else { + this.currentValue.unshift(...args); + } this.notify(); } + splice(...args) { - this.currentValue.splice(...args); + if (this.safety === 'freeze') { + const next = [...this.currentValue]; + next.splice(...args); + this.currentValue = this.protect(next); + } + else { + this.currentValue.splice(...args); + } this.notify(); } + map(mapFunction) { - this.currentValue = Array.prototype.map.call(this.currentValue, mapFunction); + this.currentValue = this.protect(Array.prototype.map.call(this.currentValue, mapFunction)); this.notify(); } + filter(filterFunction) { - this.currentValue = Array.prototype.filter.call(this.currentValue, filterFunction); + this.currentValue = this.protect(Array.prototype.filter.call(this.currentValue, filterFunction)); this.notify(); } getIndex(index) { return this.get()[index]; } + setIndex(index, value) { - this.currentValue[index] = value; + if (this.safety === 'freeze') { + const next = [...this.currentValue]; + next[index] = value; + this.currentValue = this.protect(next); + } + else { + this.currentValue[index] = value; + } this.notify(); } + removeIndex(index) { - this.currentValue.splice(index, 1); + if (this.safety === 'freeze') { + const next = [...this.currentValue]; + next.splice(index, 1); + this.currentValue = this.protect(next); + } + else { + this.currentValue.splice(index, 1); + } this.notify(); } - // sets setArrayProperty(indexOrProperty, property, value) { let index; if (isNumber(indexOrProperty)) { @@ -278,16 +286,26 @@ export class Signal { value = property; property = indexOrProperty; } - if (index === -1) { - return; + if (index === -1) { return; } + + if (this.safety === 'freeze') { + const newValue = this.currentValue.map((object, currentIndex) => { + if (index === 'all' || currentIndex === index) { + return { ...object, [property]: value }; + } + return object; + }); + this.set(newValue); } - const newValue = this.peek().map((object, currentIndex) => { - if (index == 'all' || currentIndex == index) { - object[property] = value; + else { + const arr = this.currentValue; + for (let i = 0; i < arr.length; i++) { + if (index === 'all' || i === index) { + arr[i][property] = value; + } } - return object; - }); - this.set(newValue); + this.notify(); + } } toggle() { @@ -297,18 +315,15 @@ export class Signal { increment(amount = 1, max) { return this.mutate(val => { let newAmount = val + amount; - if (isNumber(max) && newAmount > max) { - newAmount = max; - } + if (isNumber(max) && newAmount > max) { newAmount = max; } return newAmount; }); } + decrement(amount = 1, min) { return this.mutate(val => { let newAmount = val - amount; - if (isNumber(min) && newAmount < min) { - newAmount = min; - } + if (isNumber(min) && newAmount < min) { newAmount = min; } return newAmount; }); } @@ -323,21 +338,26 @@ export class Signal { } return [item]; } + getID(item) { return this.getIDs(item).filter(Boolean)[0]; } + hasID(item, id) { return this.getID(item) === id; } + getItem(id) { const index = this.getItemIndex(id); if (index !== -1) { return this.getIndex(index); } } + getItemIndex(id) { return findIndex(this.currentValue, item => this.hasID(item, id)); } + setProperty(idOrProperty, property, value) { if (isArray(this.currentValue)) { const id = idOrProperty; @@ -347,15 +367,99 @@ export class Signal { else { value = property; property = idOrProperty; - const obj = this.peek(); - obj[property] = value; - this.set(obj); + if (this.safety === 'freeze') { + this.set({ ...this.currentValue, [property]: value }); + } + else { + this.currentValue[property] = value; + this.notify(); + } } } + replaceItem(id, item) { return this.setIndex(this.getItemIndex(id), item); } + removeItem(id) { return this.removeIndex(this.getItemIndex(id)); } + + /*------------------- + Tracing + --------------------*/ + + setContext(additionalContext = {}) { + if (config.mode === 'off') { return; } + const defaultContext = { + value: this.currentValue, + }; + this.context = { + ...defaultContext, + ...additionalContext, + }; + } + + addContext(additionalContext = {}) { + if (config.mode === 'off') { return; } + if (!this.context) { this.context = {}; } + for (const key in additionalContext) { + this.context[key] = additionalContext[key]; + } + } + + // Error.captureStackTrace is 10-100× a context spread; gated on stack mode. + setTrace() { + captureStack(this, this.setTrace); + } + + /*------------------- + Instance of + --------------------*/ + + static [Symbol.hasInstance](instance) { + return !!instance?.[signalTag]; + } + + get [signalTag]() { + return true; + } + + /*------------------- + Configuration + --------------------*/ + + static defaultEquality = isEqual; + static defaultClone = clone; + static noEquality = () => false; + + static get safety() { + return config.safety; + } + static set safety(preset) { + if (preset !== 'freeze' && preset !== 'reference' && preset !== 'none') { + throw new Error(`Invalid Signal.safety: ${preset}. Must be 'freeze', 'reference', or 'none'.`); + } + config.safety = preset; + } + + static get tracing() { + return config.mode !== 'off'; + } + static set tracing(enabled) { + if (enabled && config.mode === 'off') { config.mode = 'context'; } + else if (!enabled) { config.mode = 'off'; } + } + + static get stackCapture() { + return config.mode === 'stack'; + } + static set stackCapture(enabled) { + if (enabled) { config.mode = 'stack'; } + else if (config.mode === 'stack') { config.mode = 'context'; } + } + + static configure(config = {}) { + Object.assign(Signal, config); + } } diff --git a/packages/reactivity/test/unit/reaction.test.js b/packages/reactivity/test/unit/reaction.test.js index 2ea31f23a..634b7820a 100644 --- a/packages/reactivity/test/unit/reaction.test.js +++ b/packages/reactivity/test/unit/reaction.test.js @@ -1,4 +1,4 @@ -import { Reaction, Scheduler, setTracing, Signal } from '@semantic-ui/reactivity'; +import { Reaction, Scheduler, Signal } from '@semantic-ui/reactivity'; import { beforeEach, describe, expect, it, vi } from 'vitest'; describe('Reaction', () => { @@ -258,8 +258,8 @@ describe('Reaction', () => { expect(callback).toHaveBeenCalledTimes(3); }); - it('should correctly manipulate array with helpers when allowClone is false', () => { - const items = new Signal([0, 1, 2], { allowClone: false }); + it('should correctly manipulate array with helpers under safety: reference', () => { + const items = new Signal([0, 1, 2], { safety: 'reference' }); const callback = vi.fn(); Reaction.create(() => { @@ -327,7 +327,7 @@ describe('Reaction', () => { describe('Debugging', () => { it('Reaction should track current context for debugging', () => { - setTracing(true); + Signal.tracing = true; try { const callback = vi.fn(); let signal = new Signal(1); @@ -343,7 +343,7 @@ describe('Reaction', () => { expect(callback).toHaveBeenCalledWith(2); } finally { - setTracing(false); + Signal.tracing = false; } }); diff --git a/packages/reactivity/test/unit/signal.test.js b/packages/reactivity/test/unit/signal.test.js index b95a51ec8..a3a443a3d 100644 --- a/packages/reactivity/test/unit/signal.test.js +++ b/packages/reactivity/test/unit/signal.test.js @@ -530,7 +530,7 @@ describe.concurrent('Signal', () => { describe.concurrent('Cloning Behavior with Signals', () => { it('should maintain reactivity when using a Signal inside another Signal', () => { const innerCallback = vi.fn(); - const innerVar = new Signal(1, { allowClone: true }); + const innerVar = new Signal(1); const outerCallback = vi.fn(); const outerVar = new Signal(innerVar); @@ -555,8 +555,8 @@ describe.concurrent('Signal', () => { const data1 = { id: 1, text: 'test object' }; const data2 = { id: 2, text: 'test object 2' }; - const innerVar1 = new Signal(data1, { allowClone: true }); - const innerVar2 = new Signal(data2, { allowClone: true }); + const innerVar1 = new Signal(data1); + const innerVar2 = new Signal(data2); innerVar1.subscribe(innerCallback1); innerVar2.subscribe(innerCallback2); @@ -787,7 +787,7 @@ describe.concurrent('Signal', () => { obj => ({ doubled: obj.count * 2 }), { equalityFunction: (a, b) => a?.doubled === b?.doubled, - allowClone: false, + safety: 'reference', }, ); @@ -894,32 +894,35 @@ describe.concurrent('Signal', () => { }); /******************************* - Mutate Dev Warning + Mutate Safety Behavior *******************************/ - it('should warn in dev when mutate returns the same reference', () => { - const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); - const signal = new Signal([1, 2, 3]); + it('throws under freeze when mutate tries to mutate in place', () => { + const signal = new Signal([1, 2, 3], { safety: 'freeze' }); + + expect(() => { + signal.mutate(arr => { + arr.push(4); + }); + }).toThrow(TypeError); + }); + + it('allows in-place mutate under reference safety', () => { + const signal = new Signal([1, 2, 3], { safety: 'reference' }); signal.mutate(arr => { arr.push(4); - return arr; }); - expect(warnSpy).toHaveBeenCalledTimes(1); - expect(warnSpy.mock.calls[0][0]).toMatch(/same reference/); - warnSpy.mockRestore(); + expect(signal.get()).toEqual([1, 2, 3, 4]); }); - it('should not warn when mutate returns a new value', () => { - const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); - const signal = new Signal([1, 2, 3]); + it('accepts a returned new value under freeze', () => { + const signal = new Signal([1, 2, 3], { safety: 'freeze' }); signal.mutate(() => [4, 5, 6]); - expect(warnSpy).not.toHaveBeenCalled(); expect(signal.get()).toEqual([4, 5, 6]); - warnSpy.mockRestore(); }); // Test WeakRef cleanup behavior @@ -965,4 +968,69 @@ describe.concurrent('Signal', () => { expect(fake instanceof Signal).toBe(true); }); }); + + describe('Dev-mode freeze error decoration', () => { + // Sequential describe — toggling Signal.tracing is global state. + + it('returns raw frozen value when tracing is off', () => { + const signal = new Signal({ a: 1 }); + expect(signal.get()).toBe(signal.peek()); + }); + + it('returns a proxy with a framework-authored error when tracing is on', () => { + Signal.tracing = true; + try { + const signal = new Signal({ a: 1 }); + const proxy = signal.get(); + expect(proxy).not.toBe(signal.peek()); + expect(() => { + proxy.a = 2; + }).toThrow(/Signal value is frozen.*`a`/s); + expect(() => { + delete proxy.a; + }).toThrow(/Signal value is frozen/); + } + finally { + Signal.tracing = false; + } + }); + + it('caches the proxy per raw value so get() === get()', () => { + Signal.tracing = true; + try { + const signal = new Signal({ a: 1 }); + expect(signal.get()).toBe(signal.get()); + } + finally { + Signal.tracing = false; + } + }); + + it('returns raw for reference-safety signals even in dev', () => { + Signal.tracing = true; + try { + const signal = new Signal({ a: 1 }, { safety: 'reference' }); + const value = signal.get(); + expect(value).toBe(signal.peek()); + expect(() => { + value.a = 2; + }).not.toThrow(); + } + finally { + Signal.tracing = false; + } + }); + + it('returns raw for Map/Set/Date/class values (outside deepFreeze scope)', () => { + Signal.tracing = true; + try { + const map = new Map(); + const signal = new Signal(map); + expect(signal.get()).toBe(map); + } + finally { + Signal.tracing = false; + } + }); + }); }); diff --git a/packages/reactivity/types/signal.d.ts b/packages/reactivity/types/signal.d.ts index 90032f6eb..b245fe1f5 100755 --- a/packages/reactivity/types/signal.d.ts +++ b/packages/reactivity/types/signal.d.ts @@ -10,13 +10,15 @@ export interface SignalOptions { equalityFunction?: (oldValue: T, newValue: T) => boolean; /** - * Whether to allow cloning of values. If false, values are stored by reference - * @default true + * Safety preset controlling how the signal protects its value + * - 'freeze' (default) — deepFreeze on set; mutations throw + * - 'reference' — no protection; dedupe via isEqual + * - 'none' — no protection, no dedupe (event-stream semantics) */ - allowClone?: boolean; + safety?: 'freeze' | 'reference' | 'none'; /** - * Custom function to clone values when storing or retrieving from the signal + * Custom function to clone values (used by signal.clone()) * @param value - The value to clone */ cloneFunction?: (value: V) => V; @@ -130,10 +132,20 @@ export class Signal { /** * Returns the current value without establishing a reactive dependency. * Accessing the value with `peek()` will not cause any reactive context to depend on this Signal. + * Under `safety: 'freeze'`, the returned reference is frozen — use `clone()` + * if you need a mutable copy. * @see {@link https://next.semantic-ui.com/docs/api/reactivity/signal#peek peek} */ peek(): T; + /** + * Tracked read that returns a deep copy of the current value. Use when + * handing signal data to libraries that mutate in place, or when the + * signal has `safety: 'freeze'` and you need a mutable working copy. + * @see {@link https://next.semantic-ui.com/docs/api/reactivity/signal#clone clone} + */ + clone(): T; + /** * Sets the signal's value to undefined. * Triggers updates if the value was not already undefined. @@ -152,9 +164,12 @@ export class Signal { subscribe(callback: (value: T, computation: { stop: () => void; }) => void): { stop: () => void; }; /** - * Mutates the current value in-place via a callback function. - * If the callback returns a value, that value is set. Otherwise the original - * value is kept and reactivity is triggered if it changed. + * Mutates the current value via a callback function. Under `safety: 'freeze'` + * the callback receives a frozen value and must return a new value (in-place + * mutation throws). Under `safety: 'reference'` / `'none'` the callback may + * mutate in place; a returned non-undefined value replaces the current value, + * while undefined-return triggers notify when the value actually changed + * (`'reference'`) or unconditionally (`'none'`, event-stream semantics). * @see {@link https://next.semantic-ui.com/docs/api/reactivity/signal#mutate mutate} * @param mutationFn - Function that receives the current value and optionally returns a new value */ @@ -430,27 +445,45 @@ export class Signal { static computed(computeFn: () => T, options?: SignalOptions): Signal; /** - * Enables or disables tracing — cheap debug context attached to signals, - * reactions, and dependencies. Off by default. - * @param enabled - Whether to enable tracing + * Default safety preset for new signals. Assign a preset to change the + * library-wide default: `Signal.safety = 'freeze'`. + */ + static safety: 'freeze' | 'reference' | 'none'; + + /** + * Cheap debug context attached to signals, reactions, and dependencies. + * Off by default. Assign to toggle: `Signal.tracing = true`. + */ + static tracing: boolean; + + /** + * Adds stack traces to tracing context via Error.captureStackTrace. + * Expensive; opt-in on top of tracing. Assign to toggle: + * `Signal.stackCapture = true`. */ - static setTracing(enabled: boolean): void; + static stackCapture: boolean; /** - * Returns whether tracing is currently enabled. + * Default equality function used to dedupe signal writes. Assign to + * override library-wide: `Signal.defaultEquality = myEq`. */ - static isTracing(): boolean; + static defaultEquality: (a: any, b: any) => boolean; /** - * Enables or disables stack capture — adds stack traces to tracing - * context via Error.captureStackTrace. Expensive; opt-in on top of tracing. - * Off by default. - * @param enabled - Whether to enable stack capture + * Default clone function used by `signal.clone()`. Assign to override + * library-wide: `Signal.defaultClone = myClone`. */ - static setStackCapture(enabled: boolean): void; + static defaultClone: (value: V) => V; /** - * Returns whether stack capture is currently enabled. + * Bulk-configure library defaults. Equivalent to assigning each key + * individually; `safety` validates on set. */ - static isStackCapture(): boolean; + static configure(options: { + safety?: 'freeze' | 'reference' | 'none'; + tracing?: boolean; + stackCapture?: boolean; + defaultEquality?: (a: any, b: any) => boolean; + defaultClone?: (value: V) => V; + }): void; } diff --git a/packages/renderer/src/engines/lit/directives/render-template.js b/packages/renderer/src/engines/lit/directives/render-template.js index b8938a8e7..f80b5d0c9 100644 --- a/packages/renderer/src/engines/lit/directives/render-template.js +++ b/packages/renderer/src/engines/lit/directives/render-template.js @@ -142,10 +142,13 @@ export class RenderTemplateDirective extends AsyncDirective { } unpackData(dataObj) { - if (isFunction(dataObj)) { - return dataObj(); - } - return mapObject(dataObj, (val) => val()); + const raw = isFunction(dataObj) ? dataObj() : mapObject(dataObj, (val) => val()); + // Subtemplate uses the result as a mutable container for setDataContext + // merges. Signal values under safety: 'freeze' come in frozen, so + // shallow-copy plain objects and arrays on the way through. + if (raw === null || typeof raw !== 'object') { return raw; } + if (Array.isArray(raw)) { return [...raw]; } + return { ...raw }; } update(part, settings) { diff --git a/packages/renderer/src/engines/lit/renderer.js b/packages/renderer/src/engines/lit/renderer.js index e51f8950b..fb131d88f 100644 --- a/packages/renderer/src/engines/lit/renderer.js +++ b/packages/renderer/src/engines/lit/renderer.js @@ -57,7 +57,7 @@ export class LitRenderer { this.inheritsData = inheritsData; // for subtrees lets us know if this needs to have data updates downstream this.protectedKeys = protectedKeys; // keys scoped to this subtree (loop vars, async results) that parent updates cannot overwrite this.id = LitRenderer.getID({ ast, data, isSVG }); - this.dataVersion = new Signal(0, { allowClone: false, equalityFunction: () => false }); + this.dataVersion = new Signal(0, { safety: 'none' }); // Delegate expression evaluation this.evaluator = new ExpressionEvaluator({ diff --git a/packages/renderer/src/engines/native/blocks/each.js b/packages/renderer/src/engines/native/blocks/each.js index 0360dc448..71b24c705 100644 --- a/packages/renderer/src/engines/native/blocks/each.js +++ b/packages/renderer/src/engines/native/blocks/each.js @@ -218,7 +218,8 @@ function disposeRecordDOM(record) { function createRecord({ key, item, index, collectionType, node, data, scope, renderAST, isSVG }) { const eachData = getEachData(item, index, collectionType, node); const itemScope = scope.child(); - const itemSignal = new Signal(eachData, { allowClone: false }); + // Framework-internal signal over user-owned iteration data — don't freeze + const itemSignal = new Signal(eachData, { safety: 'reference' }); const itemProxy = createItemDataProxy(data, itemSignal); const fragment = renderAST({ ast: node.content, data: itemProxy, scope: itemScope, isSVG }); // Marker-bounded item range: startMarker ... [item content] ... endMarker. @@ -552,7 +553,8 @@ function adoptServerItems({ usedKeys.add(key); const eachData = getEachData(item, i, collectionType, node); const itemScope = scope.child(); - const itemSignal = new Signal(eachData, { allowClone: false }); + // Framework-internal signal over user-owned iteration data — don't freeze + const itemSignal = new Signal(eachData, { safety: 'reference' }); const itemProxy = createItemDataProxy(data, itemSignal); // Wire per-item reactivity on the existing DOM. hydrateInnerContent diff --git a/packages/renderer/src/engines/native/reaction-scope.js b/packages/renderer/src/engines/native/reaction-scope.js index 7f3bf7502..cab09a7e2 100644 --- a/packages/renderer/src/engines/native/reaction-scope.js +++ b/packages/renderer/src/engines/native/reaction-scope.js @@ -1,4 +1,4 @@ -import { isTracing, Reaction } from '@semantic-ui/reactivity'; +import { Reaction, Signal } from '@semantic-ui/reactivity'; export class ReactionScope { constructor() { @@ -21,8 +21,8 @@ export class ReactionScope { return; } callback(comp); - }, isTracing() ? { context } : undefined)); - // isTracing() prevents flamecarts from resolving as reaction.context.context.context + }, Signal.tracing ? { context } : undefined)); + // Signal.tracing gate prevents flamecharts from resolving as reaction.context.context.context } onDispose(fn) { diff --git a/packages/renderer/test/browser/ssr-hydration.test.js b/packages/renderer/test/browser/ssr-hydration.test.js index 4981beb38..8d3a6004d 100644 --- a/packages/renderer/test/browser/ssr-hydration.test.js +++ b/packages/renderer/test/browser/ssr-hydration.test.js @@ -1344,9 +1344,9 @@ describe('SSR hydration — snippet reactivity', () => { defaultState: { items: [{ name: 'Red' }, { name: 'Blue' }] }, createComponent: ({ state }) => ({ renameFirst() { - const items = state.items.peek(); + const items = [...state.items.peek()]; items[0] = { ...items[0], name: 'Crimson' }; - state.items.set([...items]); + state.items.set(items); }, }), }); diff --git a/packages/templating/src/template.js b/packages/templating/src/template.js index 7d3f0e95d..2356696bb 100644 --- a/packages/templating/src/template.js +++ b/packages/templating/src/template.js @@ -944,7 +944,8 @@ export const Template = class Template { if (property in target) { let signal = template.settingsVars.get(property); if (!signal) { - signal = new Signal(target[property], { allowClone: false }); + // Framework-internal signal over user-owned settings — don't freeze + signal = new Signal(target[property], { safety: 'reference' }); template.settingsVars.set(property, signal); } signal.get(); // track dependency @@ -962,7 +963,7 @@ export const Template = class Template { signal.set(value); } else { - signal = new Signal(value, { allowClone: false }); + signal = new Signal(value, { safety: 'reference' }); template.settingsVars.set(property, signal); } return true; diff --git a/packages/utils/src/functions.js b/packages/utils/src/functions.js index ed3964bad..720efc674 100755 --- a/packages/utils/src/functions.js +++ b/packages/utils/src/functions.js @@ -6,9 +6,14 @@ import { isFunction, isPlainObject, isPromise } from './types.js'; --------------------*/ /* - Efficient no operation func + No operation — swallows arguments, returns undefined */ -export const noop = (v) => v; +export const noop = () => {}; + +/* + Identity — returns its first argument unchanged +*/ +export const identity = (v) => v; /* Call function even if its not defined diff --git a/packages/utils/src/objects.js b/packages/utils/src/objects.js index f5ed7d94a..0a55bad77 100755 --- a/packages/utils/src/objects.js +++ b/packages/utils/src/objects.js @@ -1,5 +1,4 @@ import { clone } from './cloning.js'; -import { noop } from './functions.js'; import { each } from './loops.js'; import { escapeRegExp } from './regexp.js'; import { isArray, isObject, isPlainObject, isString } from './types.js'; @@ -38,19 +37,29 @@ export const mapObject = (obj, callback) => { }; /* - Handles properly copying getter/setters + Shallow-merge sources into target. Preserves source accessors (unlike + Object.assign, which snapshots getter values) and preserves target + extensibility (a frozen/sealed source does not lock down target props). */ export const extend = (obj, ...sources) => { sources.forEach((source) => { - let descriptor, prop; if (source) { - for (prop in source) { - descriptor = Object.getOwnPropertyDescriptor(source, prop); - if (descriptor === undefined) { - obj[prop] = source[prop]; + for (const prop in source) { + const desc = Object.getOwnPropertyDescriptor(source, prop); + if (desc?.get || desc?.set) { + Object.defineProperty(obj, prop, desc); + } + else if (prop in obj) { + // Existing target prop may be accessor from a prior source + Object.defineProperty(obj, prop, { + value: source[prop], + writable: true, + enumerable: true, + configurable: true, + }); } else { - Object.defineProperty(obj, prop, descriptor); + obj[prop] = source[prop]; } } } @@ -240,7 +249,7 @@ export const get = function(obj, path = '') { /* This is useful for callbacks or other scenarios where you want to avoid the values of a reference object becoming stale when a source object changes */ -export const proxyObject = (sourceObj = noop, referenceObj = {}) => { +export const proxyObject = (sourceObj = () => ({}), referenceObj = {}) => { return new Proxy(referenceObj, { get: (target, property) => { const propKey = typeof property === 'symbol' ? property.toString() : property; @@ -259,11 +268,10 @@ export const onlyKeys = (obj, keysToKeep) => { }; /* - Return true if non-inherited property + Return true if non-inherited property. Thin re-export of Object.hasOwn + (ES2022) — safe on Object.create(null) and objects that shadow hasOwnProperty. */ -export const hasProperty = (obj, prop) => { - return Object.prototype.hasOwnProperty.call(obj, prop); -}; +export const hasProperty = Object.hasOwn; /* Reverses a lookup object diff --git a/packages/utils/src/strings.js b/packages/utils/src/strings.js index ca61bf83d..1ac87d1a9 100755 --- a/packages/utils/src/strings.js +++ b/packages/utils/src/strings.js @@ -1,4 +1,4 @@ -import { noop } from './functions.js'; +import { identity } from './functions.js'; import { isArray, isFunction, isString } from './types.js'; /*------------------- @@ -109,7 +109,7 @@ export const joinWords = (words, { lastSeparator = ' and ', oxford = true, quotes = false, - transform = noop, + transform = identity, } = {}) => { if (!isArray(words) || words.length === 0) { return ''; diff --git a/packages/utils/test/functions.test.js b/packages/utils/test/functions.test.js index a3f2ef96c..e184f8213 100644 --- a/packages/utils/test/functions.test.js +++ b/packages/utils/test/functions.test.js @@ -1,12 +1,20 @@ -import { debounce, memoize, noop, throttle, wait, wrapFunction } from '@semantic-ui/utils'; +import { debounce, identity, memoize, noop, throttle, wait, wrapFunction } from '@semantic-ui/utils'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; describe('function utilities', () => { - it('noop should act as an identity function', () => { - expect(noop(42)).toBe(42); - expect(noop('hello')).toBe('hello'); + it('noop returns undefined and swallows arguments', () => { expect(noop()).toBeUndefined(); + expect(noop(42)).toBeUndefined(); + expect(noop('hello', 'world')).toBeUndefined(); + }); + + it('identity returns its first argument unchanged', () => { + expect(identity(42)).toBe(42); + expect(identity('hello')).toBe('hello'); + const obj = { a: 1 }; + expect(identity(obj)).toBe(obj); + expect(identity()).toBeUndefined(); }); it('wrapFunction should return the same function if a function is passed', () => { const func = () => 'test'; @@ -888,8 +896,12 @@ describe('function utilities', () => { }); describe('debounce — options object overload', () => { - beforeEach(() => { vi.useFakeTimers(); }); - afterEach(() => { vi.restoreAllMocks(); }); + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.restoreAllMocks(); + }); it('should accept options object as second argument', async () => { const func = vi.fn(() => 'result'); @@ -905,8 +917,12 @@ describe('debounce — options object overload', () => { }); describe('throttle — options object overload', () => { - beforeEach(() => { vi.useFakeTimers(); }); - afterEach(() => { vi.restoreAllMocks(); }); + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.restoreAllMocks(); + }); it('should accept options object as second argument', () => { const func = vi.fn(() => 'result'); diff --git a/packages/utils/types/functions.d.ts b/packages/utils/types/functions.d.ts index 8fefa59d8..5a57d7a83 100755 --- a/packages/utils/types/functions.d.ts +++ b/packages/utils/types/functions.d.ts @@ -77,6 +77,18 @@ export interface ThrottledFunction any> { */ export function noop(...args: any[]): void; +/** + * Identity function — returns its first argument unchanged + * @see {@link https://next.semantic-ui.com/docs/api/utils/functions#identity identity} + * + * @example + * ```ts + * const transform = mapFn ?? identity; + * const mapped = items.map(transform); + * ``` + */ +export function identity(value: T): T; + /** * Wraps a value in a function if it isn't already a function * @see {@link https://next.semantic-ui.com/docs/api/utils/functions#wrapfunction wrapFunction} diff --git a/src/components/global-search/global-search.js b/src/components/global-search/global-search.js index 81296af1d..cdd7acf2d 100644 --- a/src/components/global-search/global-search.js +++ b/src/components/global-search/global-search.js @@ -13,12 +13,16 @@ const defaultSettings = { debounceTime: 200, }; +// All pagefind-derived signals: pagefind caches fragment objects internally and mutates +// them on later .data() calls, so freeze would poison its cache on subsequent searches. +const referenceOptions = { options: { safety: 'reference' } }; + const defaultState = { - rawResults: [], - results: [], - displayResults: [], + rawResults: { value: [], ...referenceOptions }, + results: { value: [], ...referenceOptions }, + displayResults: { value: [], ...referenceOptions }, + selectedResult: { value: undefined, ...referenceOptions }, selectedIndex: 0, - selectedResult: undefined, resultOffset: 0, searchTerm: '', noResults: false, diff --git a/src/components/mobile-menu/mobile-menu.js b/src/components/mobile-menu/mobile-menu.js index 0415054e7..0c539b484 100644 --- a/src/components/mobile-menu/mobile-menu.js +++ b/src/components/mobile-menu/mobile-menu.js @@ -100,16 +100,13 @@ const createComponent = ({ tpl, settings, $, state, flush, afterFlush, dispatchE // add -> icon if the menu has a sub menu addNavIcons(menu) { - if (!settings.navIcon) { + if (!settings.navIcon || !menu?.menu) { return menu; } - (menu?.menu || []).map(item => { - if (item.menu) { - item.navIcon = settings.navIcon; - } - return item; - }); - return menu; + return { + ...menu, + menu: menu.menu.map(item => item.menu ? { ...item, navIcon: settings.navIcon } : item), + }; }, addTrailingSlash(url) { return (url.substr(-1) === '/') diff --git a/src/components/nav-menu/nav-menu.js b/src/components/nav-menu/nav-menu.js index 850b72073..1d5d85e32 100644 --- a/src/components/nav-menu/nav-menu.js +++ b/src/components/nav-menu/nav-menu.js @@ -43,7 +43,6 @@ const createComponent = ({ $, el, self, settings, state, reaction, isRendered }) menu = []; } } - menu = clone(menu) || []; menu = self.filterVisibleSections(menu); if (self.isSearching()) { menu = self.filterBySearchTerm(menu); @@ -147,25 +146,30 @@ const createComponent = ({ $, el, self, settings, state, reaction, isRendered }) let firstMatch = false; const searchTermChanged = self.lastSearchTerm !== searchTerm; const addSelectedIndex = (item) => { - if (item?.url) { - selectedIndex++; - item.selectedIndex = selectedIndex; + if (!item?.url) { + return item; } + selectedIndex++; // reset selected index only when search term changes - if (searchTermChanged && !firstMatch && item.highlight && item?.url) { + if (searchTermChanged && !firstMatch && item.highlight) { state.selectedIndex.set(selectedIndex); firstMatch = true; } - return item; + return { ...item, selectedIndex }; }; // add selected index for each menu, pages and subpages (3 deep max) menu = menu.map(currentMenu => { - currentMenu = addSelectedIndex(currentMenu); - (currentMenu?.pages || []).map(page => { - page = addSelectedIndex(page); - (page?.pages || []).map(subPage => addSelectedIndex(subPage)); + const withIndex = addSelectedIndex(currentMenu); + if (!withIndex?.pages) { return withIndex; } + const pages = withIndex.pages.map(page => { + const pageWithIndex = addSelectedIndex(page); + if (!pageWithIndex?.pages) { return pageWithIndex; } + return { + ...pageWithIndex, + pages: pageWithIndex.pages.map(subPage => addSelectedIndex(subPage)), + }; }); - return currentMenu; + return { ...withIndex, pages }; }); self.lastSearchTerm = searchTerm; state.maxIndex.set(selectedIndex); diff --git a/tools/benchmark/src/main.js b/tools/benchmark/src/main.js index 2ba93247c..e96f50f5f 100644 --- a/tools/benchmark/src/main.js +++ b/tools/benchmark/src/main.js @@ -5,8 +5,8 @@ import { buildData } from './store.js'; import ast from './template.html?ast'; const defaultState = { - rows: { value: [], options: { allowClone: false, equalityFunction: () => false } }, - selected: { value: 0, options: { allowClone: false, equalityFunction: () => false } }, + rows: { value: [], options: { safety: 'none' } }, + selected: { value: 0, options: { safety: 'none' } }, }; const createComponent = ({ state }) => ({ diff --git a/tools/lsp/src/component-analyzer.js b/tools/lsp/src/component-analyzer.js index 100e54601..535cace82 100644 --- a/tools/lsp/src/component-analyzer.js +++ b/tools/lsp/src/component-analyzer.js @@ -224,6 +224,9 @@ function findReturnExpression(block) { /* Extracts keys and inferred types from an object literal node. + State entries may be declared as a raw value (`count: 0`) or as a + signal config object (`rows: { value: [], options: {...} }`); in the + latter case, infer from the inner `value` node. */ function extractObjectFields(node) { const fields = []; @@ -234,16 +237,29 @@ function extractObjectFields(node) { const name = getPropertyName(prop); if (!name) { continue; } + const valueNode = unwrapSignalConfig(prop.value); fields.push({ name, - inferredType: inferTypeFromValue(prop.value), - defaultValue: getLiteralValue(prop.value), + inferredType: inferTypeFromValue(valueNode), + defaultValue: getLiteralValue(valueNode), }); } return fields; } +function unwrapSignalConfig(node) { + if (!node || node.type !== 'ObjectExpression') { return node; } + const props = node.properties.filter(p => p.type === 'Property'); + const keys = props.map(getPropertyName); + if (!keys.includes('value')) { return node; } + const hasOptions = keys.includes('options') + || node.properties.some(p => p.type === 'SpreadElement'); + if (!hasOptions) { return node; } + const valueProp = props.find(p => getPropertyName(p) === 'value'); + return valueProp?.value ?? node; +} + function extractEventKeys(node) { const keys = []; if (!node || node.type !== 'ObjectExpression') { return keys; }