react icon indicating copy to clipboard operation
react copied to clipboard

[React 19] custom element property vs. event handler for props beginning with "on"

Open cjpillsbury opened this issue 1 year ago • 3 comments

Summary

Per discussion in issue #11347 and in PR #22184, the current implementation of custom element support in React 19 does the following:

  1. checks if the react prop name begins with on
  2. checks if the react prop value is of type function

If both conditions hold, it will addEventListener for an event based off of the remaining name, also using naming conventions to check if the suffix is Capture to determine whether or not to use capture phase for the event.

Example:

<my-custom-element
  // This will listen for events of type `"customevent"`
  oncustomevent={(evt) => console.log(evt.type)}
  // This will listen for events of type `"customevent"` and will `useCapture`(See: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#usecapture)
  oncustomeventCapture={(evt) => console.log(event.eventPhase, event.type)}
/>

In the aforementioned discussions, there was some debate on how to approach events, namely:

I originally proposed that we do the same thing as preact here (forward everything starting with "on" to addEventListener), but if I remember correctly Sebastian felt strongly that we shouldn't reserve everything starting with "on" for event listeners. Jason Miller also felt strongly about this the other way, so maybe it's something that I can revisit/discuss more after merging this PR - maybe more users will chime in if they get to test it out.

  • https://github.com/facebook/react/pull/22184#issuecomment-987510813 from @josepharhar

Referencing this, @jfbrennan called out, among other things, the relevant bits of the HTML Standard (See: https://github.com/facebook/react/issues/11347#issuecomment-1767301646)

I believe there is a simple path forward to strike a balance here while still having support that is "idiomatically React"

Proposal

Prefer the "in heuristic" even for properties that meet the two conditions mentioned above over adding an event listener.

Motivation

  • For cases that conform to the HTML standard for custom element properties/IDL attributes, a property intended to handle events should behave the same as the current implementation: namely, if I (or react-dom) set myCustomElement.oncustomevent = (evt) => console.log(evt.type);, this should add an event listener "under the hood"
  • For cases where custom element authors have built properties that happen to begin with "on" and happen to accept functions as values, these will still work as expected, e.g. if there is a property on my-custom-element named "onto" it will get set as expected, even if the value is a function, so, e.g.
<my-custom-element
  // This will be set as the value of property `"onto"`, even though it begins with `"on"` and its value is of type `function`
  onto={(value) => console.log(value)}
/>

Current workarounds

  • Use hooks (e.g. useRef() + useEffect(), etc.) to set these properties
  • Expect web component authors to have a 2nd property whose name does not begin with "on" that mirrors the functionality of the property whose name does begin with "on".

Relevant code

https://github.com/facebook/react/blob/main/packages/react-dom-bindings/src/client/DOMPropertyOperations.js#L194-L222

cc @jakearchibald @rickhanlonii for visibility

cjpillsbury avatar May 29 '24 20:05 cjpillsbury

Oh also worth mentioning, I'm happy to issue a PR for these changes if folks are on board.

cjpillsbury avatar May 29 '24 20:05 cjpillsbury

The most ideal solution would be if JSX were updated to have syntax that lets users be 100% in control. It would be similar to Lit's html syntax (in effect, not necessarily the with matching the same syntax), which gives users the ability to pick whether a prop is setting an attribute, JS property, boolean attribute, or event listener.

In Lit, it is like this:

return html`
  <some-el
    foo="this sets the foo attribute (no guessing)"
    .bar="this sets the bar JS property (no guessing)"
    ?baz=${someBool/*this adds or removes the baz attribute based on a boolean (no guessing)*/}
    @lorem=${() => {/*this adds a listener for the lorem event (no guessing)*/}}
  ></some-el>
`

In other JSX frameworks, the following syntax features have been established:

return <some-element
  foo={/*may use heuristics to set a foo attribute or foo JS property*/}
  attr:bar={/*set the bar attribute (no guessing)*/}
  prop:baz={/*set the baz JS property (no guessing)*/}
  bool:boom={/*adds or remove the boom attribute based on a boolean (no guessing)*/}
  on:some-event={/*add a listener for some-event (no guessing)*/}
/>

For example, see Pota by @titoBouzout which provides that syntax for its alternative JSX compiler for Solid.js. Pota also provides an html template tag with the same syntax. Additionally, Pota's JSX and html templates both support Lit's .foo, ?foo, and @foo syntaxes.

Solid's JSX has attr:, prop:, and on:, but it is currently missing bool:.

The above approach would make it possible for users to be able to do what they want to do 100% of the time, with no guessing, by being able to select the syntax they need for the particular element.

There is no way that a heuristic-based approach can allow 100% of user cases to be satisfied, but with a fairly simple syntax addition, all users could be satisfied.

Trying to make the heuristics-based approach cover everyone's needs is, most likely, impossible.


Another way to look at it: instead of having rules that would make elements have to conform to React, allow React templating to freely express all that is possible with elements.

trusktr avatar Aug 19 '24 00:08 trusktr

Expect web component authors to have a 2nd property whose name does not begin with "on" that mirrors the functionality of the property whose name does begin with "on".

In other words, expect authors to define valid APIs, which is the right approach. Frameworks should not start supporting invalid attribute/property naming. "on" always means event handler. WC authors need to know that and follow that regardless of tech stack. As for syntax, JSX doesn't need to do anything different than it already does.

jfbrennan avatar Aug 21 '24 15:08 jfbrennan

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

github-actions[bot] avatar Nov 19 '24 16:11 github-actions[bot]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

github-actions[bot] avatar Nov 26 '24 16:11 github-actions[bot]