fix(Checkbox): preserve native event APIs in onChange compat layer by singkia · Pull Request #415 · cloudflare/kumo

singkia · 2026-04-15T16:01:52Z

What

Fix the deprecated onChange compatibility path on Checkbox so it no longer crashes on read-only Event.target and still preserves native event APIs.

Why

Object.assign(eventDetails.event, { target: { checked: newChecked } }) tries to write to Event.target, which is a read-only getter on native DOM events and crashes in modern browsers.

The first fix using Object.create(eventDetails.event, { target: ... }) avoided that write, but it also stopped passing a real native event object through the deprecated onChange path. Brand-checked APIs like preventDefault(), defaultPrevented, and timeStamp could then break.

This version proxies the original event and only overrides target.checked and currentTarget.checked, while binding the rest of the event API back to the native event object.

Test Results

Added a focused unit test for the deprecated onChange compatibility path that verifies:

target.checked and currentTarget.checked reflect the new value
native event APIs like preventDefault(), defaultPrevented, and timeStamp still work

Commands run:

pnpm --filter @cloudflare/kumo exec vitest run src/components/checkbox/checkbox.test.tsx
pnpm --filter @cloudflare/kumo exec tsc --noEmit
pnpm --filter @cloudflare/kumo test (still hits the pre-existing tests/imports/export-path-validation.test.ts failures for table-of-contents dist artifacts)

Fixes #409

AI disclosure: Claude Opus 4.6 (via Amp) authored the original patch; GPT 5.4 (via Codex) updated the fix after review to preserve native event semantics and added regression coverage.

Reviews
bonk has reviewed the change
automated review not possible because: external contributor without access to bonk
Tests
Tests included/updated

…rget Object.assign(event, { target: ... }) throws TypeError because Event.target is a read-only getter. Use Object.create to shadow the getter with an own property instead. Fixes cloudflare#409 Amp-Thread-ID: https://ampcode.com/threads/T-019d91d1-836e-718b-a3d0-4ed568514ccd Co-authored-by: Amp <amp@ampcode.com>

mattrothenberg · 2026-04-15T18:28:13Z

/bigbonk

ask-bonk

Overall this is a solid fix for a real bug. The Proxy-based approach is the right call here — it preserves brand-checked native event APIs (like preventDefault()) while overriding target.checked and currentTarget.checked for backwards compat. A few notes below.

Summary:

The core brandSafeProxy + createLegacyCheckboxChangeEvent approach is correct and well-structured.
The test covers the important behaviors.
One real issue: the changeset description is stale — it mentions Object.create but the actual implementation uses Proxy.
A couple of minor observations about the Proxy's trap coverage.

ask-bonk · 2026-04-15T18:30:39Z

+The deprecated `onChange` handler used `Object.assign(event, { target: ... })` which throws
+`TypeError: Cannot set property target of #<Event> which has only a getter` because `Event.target`
+is a read-only getter property. Replaced with `Object.create` to create a new object that shadows
+the prototype getter with an own `target` property.


Same issue — the body still describes the Object.create approach. Should be updated to reflect the Proxy implementation:

Suggested change

The deprecated `onChange` handler used `Object.assign(event, { target: ... })` which throws

`TypeError: Cannot set property target of #<Event> which has only a getter` because `Event.target`

is a read-only getter property. Replaced with `Object.create` to create a new object that shadows

the prototype getter with an own `target` property.

The deprecated `onChange` handler used `Object.assign(event, { target: ... })` which throws

`TypeError: Cannot set property target of #<Event> which has only a getter` because `Event.target`

is a read-only getter property. Replaced with a `Proxy` that intercepts property access to override

`target.checked` and `currentTarget.checked` while preserving native event APIs.

ask-bonk · 2026-04-15T18:30:39Z

+      const value = Reflect.get(target, property, target);
+      return typeof value === "function" ? value.bind(target) : value;
+    },
+  });


Minor note: this Proxy only defines a get trap. That means set operations (e.g. event.someProperty = value) will fall through to the original target object, which is the native Event. This is probably fine for this deprecated compatibility path — consumers should only be reading from the event — but worth being aware of.

If you wanted to be maximally defensive, you could add a set trap that writes to the overrides object instead. Not blocking, just flagging.

ask-bonk · 2026-04-15T18:30:39Z

+        return overrides[property];
+      }
+
+      const value = Reflect.get(target, property, target);


Good call on .bind(target) here — this is what preserves brand-checked methods like preventDefault() and stopPropagation() since they perform internal slot checks on this.

ask-bonk · 2026-04-15T18:30:40Z

+    expect(event).toBeDefined();
+    expect(event.target.checked).toBe(true);
+    expect(event.currentTarget.checked).toBe(true);
+    expect(event instanceof Event).toBe(true);


Nice — this assertion is the key one that validates the Proxy preserves the prototype chain for instanceof checks. The Proxy transparently delegates this because instanceof uses Symbol.hasInstance / prototype checks on the target.

pkg-pr-new · 2026-04-15T18:30:46Z

npm i https://pkg.pr.new/@cloudflare/kumo@415

commit: fd9ae7b

Co-authored-by: mattrothenberg <mattrothenberg@users.noreply.github.com>

ask-bonk · 2026-04-15T18:30:52Z

Review submitted. Here's a summary of my findings:

The fix is correct and well-implemented. The Proxy-based approach properly solves the TypeError: Cannot set property target of #<Event> which has only a getter crash in the deprecated onChange compatibility path while preserving native event semantics.

Key feedback:

Stale changeset description (actionable): The .changeset/fix-checkbox-onchange-target.md still describes an Object.create approach from an earlier iteration, but the actual code uses Proxy. I left suggestions to update the wording.
Proxy set trap (informational): The brandSafeProxy only defines a get trap. Writes would fall through to the native Event object. Noted as non-blocking since this is a deprecated read-only compatibility path.
Positive notes: The .bind(target) on methods correctly preserves brand-checked native APIs, and the instanceof Event test assertion validates prototype chain transparency through the Proxy.

The test passes, and there are no new type errors introduced by this change.

github run

Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>

mattrothenberg · 2026-04-15T18:32:17Z

/bigbonk will this crash in older browsers? we have to support a LOT of browsers

mattrothenberg · 2026-04-15T18:33:52Z

@singkia If I were to tell you that we're planning on cutting a major release of Kumo that includes breaking changes (e.g., getting rid of onChange altogether) would that change your approach? Is this change worth it in light of that? I appreciate you taking a look at this!

ask-bonk

Good fix for a real crash. The Proxy approach is well-structured and the test provides solid regression coverage. A few observations beyond what bonk already flagged:

Summary

The core brandSafeProxy + createLegacyCheckboxChangeEvent approach correctly solves the TypeError on Event.target while preserving brand-checked native APIs.
The test covers the critical behaviors (target.checked, currentTarget.checked, preventDefault(), instanceof Event).
The stale changeset body (mentions Object.create instead of Proxy) should be fixed per bonk's suggestions.
A couple of additional observations below.

ask-bonk · 2026-04-15T18:34:31Z

+      }
+
+      const value = Reflect.get(target, property, target);
+      return typeof value === "function" ? value.bind(target) : value;


Nit: value.bind(target) is called on every property access, creating a new bound function each time. For a deprecated compat path that fires once per click this is perfectly fine, but worth noting that the proxied event is not referentially stable for method lookups (i.e. event.preventDefault !== event.preventDefault). Not blocking — just documenting the trade-off.

ask-bonk · 2026-04-15T18:34:31Z

+): T {
+  return new Proxy(source, {
+    get(target, property) {
+      if (Object.hasOwn(overrides, property)) {


Minor: Object.hasOwn(overrides, property) works correctly for symbol keys too (since overrides is Record<PropertyKey, unknown>), which is good — this means the proxy won't accidentally intercept Symbol.toPrimitive or similar unless explicitly overridden.

ask-bonk · 2026-04-15T18:34:31Z

+  checked: boolean,
+): React.ChangeEvent<HTMLInputElement> {
+  const target = brandSafeProxy(
+    (event.target ?? {}) as EventTarget & HTMLInputElement,


The event.target ?? {} fallback means that if event.target is null (which can happen for programmatically constructed events), the proxy wraps a plain object {} instead. That's reasonable — consumers accessing target.checked will still get the correct value from the override, and other HTMLInputElement properties will just be undefined rather than throwing. Fine for a deprecated path.

ask-bonk · 2026-04-15T18:34:31Z

+      />,
+    );
+
+    expect(() => fireEvent.click(screen.getByRole("checkbox"))).not.toThrow();


Consider also testing the unchecked → checked → unchecked round-trip to verify target.checked reflects false on the second click. This would catch any stale closure issues if the proxy construction were accidentally memoized in the future.

Non-blocking, but would strengthen the test.

ask-bonk · 2026-04-15T18:34:42Z

Review submitted successfully. Here's a summary of my findings:

Review Summary for PR #415

Overall: This is a solid fix for a real bug. The Proxy-based approach correctly solves the TypeError: Cannot set property target of #<Event> crash while preserving native event semantics on the deprecated onChange path.

Key points from my review:

Stale changeset (already flagged by bonk): The .changeset/fix-checkbox-onchange-target.md body still mentions Object.create from an earlier iteration, but the code uses Proxy. Should be updated.
.bind(target) on every access: Each property access creates a new bound function, so event.preventDefault !== event.preventDefault. Not a problem for this deprecated one-shot compat path, but documented as a trade-off.
event.target ?? {} fallback: Reasonable defensive coding — if event.target is null (programmatically constructed events), the proxy wraps {} and consumers still get the correct checked value from the override.
Test suggestion: A round-trip test (checked → unchecked) would strengthen regression coverage against future stale-closure bugs. Non-blocking.

The test passes, no new type errors are introduced (the only tsc error is the pre-existing ai/schemas missing module which is auto-generated at build time).

github run

singkia mentioned this pull request Apr 15, 2026

Checkbox: Object.assign on read-only Event.target crashes onCheckedChange #409

Open

fix(checkbox): preserve native event APIs in onChange compat layer

30d7083

singkia changed the title ~~fix(Checkbox): use Object.create to avoid crash on read-only Event.target~~ fix(Checkbox): preserve native event APIs in onChange compat layer Apr 15, 2026

refactor(checkbox): simplify onChange compat proxy

fd9ae7b

ask-bonk bot reviewed Apr 15, 2026

View reviewed changes

Fix is solid; changeset description is stale.

d3e4ac9

Co-authored-by: mattrothenberg <mattrothenberg@users.noreply.github.com>

mattrothenberg and others added 2 commits April 15, 2026 14:31

Update .changeset/fix-checkbox-onchange-target.md

a2ecfe2

Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>

Merge branch 'main' into fix/checkbox-onchange-readonly-target

b5e51ac

ask-bonk bot reviewed Apr 15, 2026

View reviewed changes

Conversation

singkia commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Test Results

Uh oh!

mattrothenberg commented Apr 15, 2026

Uh oh!

ask-bonk bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ask-bonk bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

pkg-pr-new bot commented Apr 15, 2026

Uh oh!

ask-bonk bot commented Apr 15, 2026

Uh oh!

mattrothenberg commented Apr 15, 2026

Uh oh!

mattrothenberg commented Apr 15, 2026

Uh oh!

ask-bonk bot left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

ask-bonk bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk bot commented Apr 15, 2026

Review Summary for PR #415

Key points from my review:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

singkia commented Apr 15, 2026 •

edited

Loading