Conversation
`signal.value` now reads through `get`, so it subscribes the current
observer exactly as `Signal.get(signal)` does. This lets a signal be
used directly inside an expression:
let label = "theme-" ++ theme.value
The stored value moves to a `raw` field. Everything inside the package
reads and writes that; `value` is an accessor over it, and keeping the
two apart is what stops the accessor recursing into itself. `value` is
not mutable, so `signal.value = x` still does not compile and no write
can skip the scheduler.
The accessor lives on a prototype rather than on each instance. That
is not cosmetic: an own accessor moves the object into dictionary
mode, which costs every other field access on the signal, including
`subs` and `raw` on the hot `get` path. Measured on this package's
benchmark, per-instance was 3x slower reads and 18x slower creation;
on the prototype every instance keeps one hidden class.
create 10k signals 19.8ms before 23.9ms after
get 1k x100 10.2ms before 10.4ms after
set 1k x100 11.5ms before 12.0ms after
create 5k computeds 22.4ms before 25.0ms after
Five tests cover the accessor: reading, seeing a write, subscribing an
effect, refreshing a stale computed, and staying untracked inside
`untrack`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LGJYMRxSMa4uSvmr4DjJyi
ReScript Signals benchmark: PR vs mainCompared implementations:
Overall:
Per-test delta (lower is better):
Note: single-machine run in CI. Numbers can vary with runner load and Node/V8 version. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe9798f777
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /* Where the value is actually stored. Every read and write *inside* this | ||
| package goes here; `value` below is an accessor installed over it, and | ||
| keeping the two apart is what stops the accessor recursing into itself. */ | ||
| mutable raw: 'a, |
There was a problem hiding this comment.
Signal.t is exported as an open record, so downstream ReScript consumers can write signal.raw = next. Unlike Signal.set, that mutation neither increments subs.version nor calls Scheduler.notifySubs, leaving dependent effects and computeds stale. This new mutable public field therefore still permits writes that bypass the scheduler; make the backing representation opaque or otherwise prevent external access to raw.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked this, and it's pre-existing rather than introduced here — main already has:
type t<'a> = {
id: int,
mutable value: 'a,
...
}with no .resi anywhere in the package (find . -name '*.resi' returns nothing), so signal.value = next already bypasses Scheduler.notifySubs on main today. This PR renames that storage to raw; the escape hatch moves, it doesn't appear.
If anything the surface narrows: value is now non-mutable, so the assignment a consumer would reach for by accident — signal.value = x — stops compiling. Getting past the scheduler now requires deliberately writing signal.raw.
On the suggested remedy: making the representation opaque is mutually exclusive with the feature. count.value typechecking in consumer code requires the record to be public, and ReScript has no per-field privacy — an interface file has to restate the record type exactly, so it can't expose value while hiding raw. I also tried @deprecated on the field as a softer mitigation:
@deprecated("Internal storage. Use Signal.get / Signal.set.") mutable raw: 'a,It compiles clean and produces no warning at an external use site, so it's inert here — not a usable middle ground.
That leaves the tradeoff as stated in the PR's "Open question": a readable .value field costs a visible record. Closing it properly means an abstract t with .value dropped, which is this PR reverted. Leaving the thread open for you to make that call.
Generated by Claude Code
Reactivity benchmark: PR vs top frameworksCompared implementations:
Overall ranking (lower total ms is better):
Per-test runtime (ms):
Note: single-machine run in CI. Numbers can vary with runner load and Node/V8 version. |
The benchmark numbers above are a harness artifact, not a regressionThe bot reported
Repeated twice more: -66.7% / -66.5% on The reason is that Swapping the order and adding a third slot makes it unambiguous — the assignment of implementation to slot is irrelevant: Same positions, 8 ms vs 17 ms, and in the first run the PR is the fast one. What the change actually costsOne framework per process, five runs each (min-of-10 within each run):
The harness is worth fixing separatelyThe in-process design will keep producing ±68% false deltas on every PR that touches this package. The fix is to run each implementation in its own Generated by Claude Code |
|
[claude] Need to think more about the trade-offs for this one. List a summary of pros and cons, weighting the performance impact, bundle size, complexity added to the lib and its ecosystem, API simplification (read it as less ambiguity for humans and AI agents), and any other interesting metrics I might have missed for maintaining and consuming the lib. |
Trade-offsMeasured rather than estimated where I could. The headline: performance and bundle are non-issues; the real cost is that Performance — neutralOne framework per process, 5 runs each, min-of-10 within each (the in-process A/B the CI job uses is unreliable, see my comment above):
Flat. The prototype accessor costs nothing because every instance keeps one hidden class. Weight: zero. This was the risk I expected to dominate and it doesn't. Bundle — negligible
200 bytes gzipped for the whole package. Weight: near zero, unless the pitch is "zero-dependency and tiny," where a 4% gzip bump on the reactive core is worth a shrug. Complexity — small but sharpNine sites across two files must read Weight: moderate. Not the line count, the invariant. API simplification — genuinely good, with one asymmetry
But the read API becomes two shapes with different capabilities:
There's no What I'd add to your list:
|
| Axis | Verdict |
|---|---|
| Performance | Neutral — measured flat |
| Bundle | Neutral — +204 B gzip |
| Complexity | Moderate — 9 sites under a recursion invariant |
| API ergonomics | Positive — real win in compound expressions, real win for agents |
| Read-API coherence | Negative — field form exists only for the tracking read |
| Side-effect transparency | Negative — property reads and shape tests now subscribe |
| Encapsulation | Negative — record goes public, bypass renames rather than closes |
The case for merging is ergonomics; the case against is that it trades a visible cost (Signal.get(...) is noisy but honest) for an invisible one (a field read that subscribes). Performance and size, the two things that looked like blockers, are not.
If it helps break the tie: the isSignalLike hazard is fixable in Xote in one line ("value" in v), and the same fix generalises — detect with in, never with a read. That's a documentable rule. The read-API asymmetry has no such fix.
Generated by Claude Code
#39 moved the signal record into `Core.cell` so the scheduler can write a computed's result directly, and collapsed `Computed.makeWithEquals` into `makeWith(~equals, ~deferEffectsUntilRecompute)`. Both sides touched the same record, so the resolution has to hold two invariants at once: - `Core.cell` carries the `raw`/`value` split, since `Signal.t` is that type. The scheduler writes `raw`: writing `value` would recurse into the getter, and reading it during a recompute would subscribe the computed to itself. - `Computed` builds through `Signal.makeRecord` rather than a record literal. The `value` accessor lives on the prototype that constructor installs, so a literal would produce a signal whose `.value` is undefined. The per-computed `recompute` closures and the `signalRef` indirection this branch carried are gone with `makeWithEquals` — the scheduler stores the value and bumps the version itself now, which is the point of #39. Suites green: 69 in `rescript-signals`, 17 in `rescript-signals-react`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGJYMRxSMa4uSvmr4DjJyi
Re-measured after the merge — and a correction to what I said earlierThe benchmark comment re-ran on the merged head ( First, the correction. In my trade-off comment above I wrote "Performance — neutral. Weight: zero" and attributed the CI numbers to a harness ordering artifact. That control only covered
So for kairo the harness is reasonably stable, and "ordering artifact" does not explain the +10–26%. My earlier framing was too broad — it held for What isolation actually showsMedians of 3 runs, one framework per process:
Every test is equal or slightly faster. The uniform small negative sign across all nine suggests machine drift rather than a real win — the honest reading is no regression, not "6% faster".
The strongest evidence is the two CI runs against each otherSame branch, essentially the same change, two runs:
What I can't explain, stated plainlyThe overall figure landed at +7.42% and +7.61% — twice, which is more consistent than the per-test numbers. I cannot reproduce it here and I cannot rule out a small diffuse cost that only shows on the CI runner's hardware or Node build. What I can say is that no individual test reproduces a regression in isolation, and the per-test attribution in that comment is not trustworthy. Until the harness runs each implementation in its own process (and with Generated by Claude Code |
2 participants
What
signal.valuenow reads throughget, so it subscribes the current observer exactly asSignal.get(signal)does:Why
Reading a signal currently means a function call, which is fine on its own but awkward the moment the value sits inside a larger expression. This is the same ergonomic Preact signals have, and it is the one shape ReScript can express, since record field access compiles to plain property access.
The motivating case is Xote's
@xote.component, where a signal handed straight to the view is already resolved at runtime, but a signal inside a compound expression (class={"theme-" ++ theme}) is a type error. With this,class={"theme-" ++ theme.value}typechecks and stays reactive, with no type knowledge needed in the PPX.How
The stored value moves to a
rawfield. Everything inside the package reads and writes that;valueis an accessor over it. Keeping the two apart is what stops the accessor recursing into itself — this is the one correctness rule for anyone touchingSignal.resafterwards.valueis not mutable, sosignal.value = xstill does not compile and no write can bypass the scheduler.The accessor lives on a prototype, deliberately
An own accessor per instance moves the object into dictionary mode, which costs every other field access on the signal —
subsandrawon the hotgetpath included. I measured both:Per-instance was 18x slower creation and 3.5x slower reads. On the prototype every instance keeps one hidden class and the getter is free. The constructor is built once on first use and closed over inside the raw block, so nothing leaks to a global.
Tests
Five new cases in
SignalTests.res: reading, seeing a write, subscribing an enclosing effect, refreshing a stale computed, and staying untracked insideuntrack. Full suite is green — 68 inrescript-signals, 17 inrescript-signals-react.One thing worth knowing, unrelated to this change
While checking this against Xote, one Xote test (
View.render disposes its computed when the component unmounts) fails onmainas it stands today, with or without this PR. Xote pins 3.1.0, and something in the detach/settle work since then changed that behaviour. Worth a look before the next release, but it is not caused by this branch.Open question
Exposing
valuemeans the record fields are visible to consumers that re-export the type. Xote currently keepsSignal.tabstract, so it needs a matching decision on its side about how much of the record to expose. That is a Xote change, not this one.🤖 Generated with Claude Code
https://claude.ai/code/session_01LGJYMRxSMa4uSvmr4DjJyi
Generated by Claude Code