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/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/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-safety-v2.md b/ai/workspace/plans/signal-safety-v2.md new file mode 100644 index 000000000..eedf7f7f3 --- /dev/null +++ b/ai/workspace/plans/signal-safety-v2.md @@ -0,0 +1,262 @@ +--- +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: + +```js +const devProxyCache = new WeakMap(); + +get value() { + this.depend(); + const val = this.currentValue; + if (config.mode === 'off' || val === null || typeof val !== 'object') { + 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: + +- `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) + +Migrate each to `{ safety: 'reference' }` or `{ safety: 'none' }` based on the original intent. + +**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. + +## 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-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/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/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..f4a290278 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: 'reference', }; -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..9d7466cd6 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) { 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..8ce0f42bb 100755 --- a/packages/reactivity/src/signal.js +++ b/packages/reactivity/src/signal.js @@ -1,9 +1,8 @@ import { clone, + deepFreeze, findIndex, isArray, - isClassInstance, - isDevelopment, isEqual, isNumber, isObject, @@ -12,177 +11,75 @@ import { } 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'); - export class Signal { - get [IS_SIGNAL]() { - return true; - } - static [Symbol.hasInstance](instance) { - return !!instance?.[IS_SIGNAL]; - } + constructor(initialValue, options = {}) { + const { context, safety, equalityFunction, cloneFunction, allowClone } = 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 ?? (allowClone === false ? 'reference' : 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, - }; - } - - // add context to signal - addContext(additionalContext = {}) { - if (!isTracing()) { - return; - } - if (!this.context) { - this.context = {}; - } - for (const key in additionalContext) { - this.context[key] = additionalContext[key]; - } - } + /*------------------- + Core + --------------------*/ - // 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); + protect(value) { + if (value === null || typeof value !== 'object') { return value; } + return this.safety === 'freeze' ? deepFreeze(value) : value; } - static equalityFunction = isEqual; - static cloneFunction = clone; - static setTracing = setTracing; - static isTracing = isTracing; - static setStackCapture = setStackCapture; - static isStackCapture = isStackCapture; - get value() { - // Record this Signal as a dependency if inside a Reaction computation this.depend(); - const value = 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; - } - - canCloneValue(value) { - return (this.allowClone === true && !isClassInstance(value)); - } - - maybeClone(value) { - if (!this.canCloneValue(value)) { - return value; - } - if (isArray(value)) { - return value.map(value => this.maybeClone(value)); - } - return this.clone(value); + return this.currentValue; } set value(newValue) { if (!this.equalityFunction(this.currentValue, newValue)) { - this.currentValue = this.maybeClone(newValue); + this.currentValue = this.protect(newValue); this.notify(); } } - get({ clone = true } = {}) { - if (!clone) { - this.depend(); - return this.currentValue; - } + get() { return this.value; } set(newValue) { - // equality check in setter this.value = newValue; } - subscribe(callback) { - return Reaction.create((comp) => { - callback(this.value, comp); - }); + peek() { + return this.currentValue; } - // 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 - 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 - return; - } - const result = computeFn(source.get()); - derivedSignal.set(result); - }); - - // Store reaction reference for potential cleanup - derivedSignal._derivedReaction = reaction; - - return derivedSignal; + clone() { + this.depend(); + return this.cloneFunction(this.currentValue); } - // 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); + subscribe(callback) { + return Reaction.create((comp) => { + callback(this.value, comp); }); - - // Store reaction reference for potential cleanup - computedSignal._computedReaction = reaction; - - return computedSignal; } depend() { @@ -190,8 +87,6 @@ export class Signal { } 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); @@ -201,73 +96,140 @@ export class Signal { 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); + /*------------------- + Complex + --------------------*/ - 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.', - ); + static computed(computeFn, options = {}) { + const computedSignal = new Signal(undefined, options); + const reaction = Reaction.create(() => { + computedSignal.set(computeFn()); + }); + computedSignal._computedReaction = reaction; + return computedSignal; + } + + derive(computeFn, options = {}) { + const derivedSignal = new Signal(undefined, options); + // WeakRef lets the derived reaction self-stop when the source is GC'd + const sourceRef = new WeakRef(this); + + const reaction = Reaction.create(() => { + const source = sourceRef.deref(); + if (!source) { + reaction.stop(); + return; } - // 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)) { + const result = computeFn(source.get()); + derivedSignal.set(result); + }); + + derivedSignal._derivedReaction = reaction; + + return derivedSignal; + } + + /*------------------- + Mutation Helpers + --------------------*/ + + 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; + } + 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 +240,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 +269,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 +292,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 +321,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..3f63d349d 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 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/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..8a30b8a52 100644 --- a/packages/renderer/src/engines/native/blocks/each.js +++ b/packages/renderer/src/engines/native/blocks/each.js @@ -218,7 +218,7 @@ 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 }); + const itemSignal = new Signal(eachData); 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 +552,7 @@ function adoptServerItems({ usedKeys.add(key); const eachData = getEachData(item, i, collectionType, node); const itemScope = scope.child(); - const itemSignal = new Signal(eachData, { allowClone: false }); + const itemSignal = new Signal(eachData); 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..c21b36cbf 100644 --- a/packages/templating/src/template.js +++ b/packages/templating/src/template.js @@ -944,7 +944,7 @@ export const Template = class Template { if (property in target) { let signal = template.settingsVars.get(property); if (!signal) { - signal = new Signal(target[property], { allowClone: false }); + signal = new Signal(target[property]); template.settingsVars.set(property, signal); } signal.get(); // track dependency @@ -962,7 +962,7 @@ export const Template = class Template { signal.set(value); } else { - signal = new Signal(value, { allowClone: false }); + signal = new Signal(value); 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/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 }) => ({