Claim the props a component declares as its own, and add the working agreement - #11
Merged
Merged
Conversation
The runtime already routes declared props away from the root element:
`behaviour` names them, `routedKeys` excludes them from `passthrough`, and
the root slot spreads only what is left. The compiler never populated it, so
for every compiled component the list was empty and each of its own props was
also spread onto the root.
For most props that is invisible: a stray attribute on a wrapper. For a prop
whose name is a DOM event it is not. A component declaring
onInput?: (value: string) => void
wires that to an inner input and calls it with the string. With the same prop
on the wrapper too, the inner event bubbles up and calls the caller's handler
a second time with the raw InputEvent. The bubbled call lands last, so that
is the one the caller sees, and the signature it was written against is a
lie. `onChange` is declared this way by eight layouts, and consumers had
already started writing `typeof value !== "string"` guards without knowing
why they were needed.
Which props are the component's own is a question the props type answers. It
is an intersection, and the two halves differ: a referenced member --
`UIBaseProps`, `JSX.ButtonHTMLAttributes` -- is inherited HTML that belongs
on the element, while an inline object literal is what this component itself
accepts and places. So the literal's keys, and only those, are emitted as
`behaviour`.
A props type that cannot be read locally, because it is imported, emits
nothing and keeps the previous behaviour. That is the safe direction: it
leaves a prop on the element rather than dropping it.
Checked against a 98-component library: of 661 declared keys the only ones a
layout does not itself place are recipe keys, which presentation claims
first, and `children`, which the layout reads through `_stable`. No HTML
attribute stops reaching an element.
Owner
Author
|
Added Only Worth calling out for review: this is a patch bump but it does change emitted behaviour. A component's own props stop being spread onto its root element, so anything that was accidentally relying on that placement moves. The audit in the PR description found nothing in a 98-component library that does, but it is the kind of change worth a second opinion on the version. |
No Python in any form: reaching for it is the tell that a step is being solved by parsing when the tool that owns the answer could just be asked. The near substitutes are ruled out too, and jq is not on macOS to begin with. CLAUDE.md imports AGENTS.md rather than copying it, so there is one source of truth and no per-vendor fork to keep in step.
`SlotAttrs` types its values as `string`, so comparing the handler by identity had no matching `toBe` overload and `tsc --noEmit` failed while `bun test` passed. `toHaveProperty(name, value)` still asserts it is the same function, and is the idiom the rest of the file already uses.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A compiled component's own props were also being spread onto its root element, so a prop the layout wires up itself was bound twice.
What happens today
The runtime already has the machinery to prevent this.
behaviournames the props a component declares,routedKeysexcludes them frompassthrough, and the root slot spreads only what is left over. The compiler never populatedbehaviour, so for every compiled component the list was empty and each of its own props landed on the root as well.For most props that is invisible, just a stray attribute on a wrapper
<div>. For a prop named after a DOM event it is not. Giventhe layout wires that to an inner
<input>and calls it with the string, which is what the signature promises. But the same prop is also on the wrapper, andinputbubbles. So the inner event travels up and calls the caller's handler a second time with the rawInputEvent. The bubbled call lands last, so that is the one the caller actually observes.onChangeis declared with a translated signature by eight layouts in@pathscale/ui, plusonInputandonSubmit. Consumers had already started working around it without knowing what they were working around:All five are
typeof value !== "string"guards onSelect'sonChange.The rule
Which props are a component's own is a question its props type already answers, and the two halves of the intersection mean different things:
A referenced member (
UIBaseProps,JSX.ButtonHTMLAttributes,IconSlotProps) is inherited surface that belongs on the element. An inline object literal is what this component itself accepts and is responsible for placing. So the literal's keys, and only those, are emitted asbehaviour.That keeps
ButtonProps, which extendsJSX.ButtonHTMLAttributes, passingonClickand the rest through to the element exactly as before.A props type that cannot be read locally, because it is imported, emits nothing and keeps today's behaviour. That is the safe direction to fail in: it leaves a prop on the element rather than dropping one.
Blast radius
Checked by regenerating a 98-component library and comparing each emitted
behaviourlist against what its layout actually reads. Of 661 declared keys, the only ones a layout does not place itself are:presentationOwnclaims beforebehaviourOwn(first claim wins), so naming them again is a no-op;children, which layouts read through_stable.children;Grid'sasviamerge({ as: "div" }, props).No HTML attribute stops reaching an element.
Tests
Three in the transform crate: declared props are claimed, inherited HTML props are not, and a non-local props type emits nothing. One in the runtime covering the actual defect, that a declared prop stays off the root slot while an undeclared handler still reaches the element.
cargo test66 pass,cargo clippyclean,cargo fmtclean, runtime suite 149 pass.Also in this PR
The repository working agreement (
AGENTS.mdplus theCLAUDE.mdimport shim), previously opened as #12 and folded in here so this repository has one pull request rather than two. It is documentation only and touches nothing the compiler change does.