-
Notifications
You must be signed in to change notification settings - Fork 47.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bypass synthetic event system for Web Component events #7901
Comments
https://github.com/webcomponents/react-integration will create a React component from a web component constructor and give you custom event support. |
As long as we only support attributes. I don't see a problem doing this for the heuristic I'd like it better if we could just pass through all props to element properties but it seems like that ship has sailed since most web components aren't designed to handle properties properly. A massive loss to the community IMO. |
It feels like that statement is clumping all web components into a single bag of poor design, which in many cases they are, but it doesn't mean you can't optimise for the ones that are designed well. A couple paradigms I'm trying to push in the community (and with SkateJS) are:
Monica Dinculescu mentioned the former and Rob Dodson the latter in their Polymer Summit talks, so I think that's something they're trying to espouse. It's unfortunate the primitives don't make this more obvious, but I think that comes with the nature of most browser built-ins these days. React not setting props, and not supporting custom events, is the reason we've had to maintain that React integration library I posted above (https://github.com/webcomponents/react-integration). It's worth looking at as a source of some patterns that are definitely working for us in production. It's also worth noting that the patterns employed there are also used in Skate's wrapper around Incremental DOM. We set props for everything we can, falling back to attributes as a last resort. In the integration lib, events have their own special convention, similar to React's. This in particular is something to pay attention to because adding event listeners if |
@treshugart Most people seem to find this counter-intuitive but my preference would be to effectively just call @staltz What do you think about that approach? |
@sebmarkbage I like that approach but I think there are a few things to consider (and you're probably already aware):
Something like the following might be a little bit more robust: Object.keys(props).forEach(name => {
if (name in element) {
// You might also need to ensure that it's a custom element because of point 1.
element[name] = props[name];
} else {
doWhatReactNormallyDoesWithAttributes();
}
}); Brainstorming just in case. Overall, I tend to agree with you that it'd be fine for React to just do
I sure hope so. I'm trying to and I know the Polymer team is trying to, as well. I think if React did do this, that it'd be a massive help, too. |
I'm not sure what exactly you're referring to, but with Polymer we'd definitely prefer setting properties to setting attributes. Which web components don't handle properties and in what way? |
@sebmarkbage I think we can definitely encourage folks to write components that support the Object.assign approach you mentioned above. As @treshugart mentioned, I spoke about doing this in my Polymer Summit talk based on our previous twitter discussion. Having easy event support in React would also be great. |
Talk is here: https://www.youtube.com/shared?ci=256yfQG-abU I also mentioned the need to dispatch events for state changes so libraries like React can revert those changes (similar to the way React handles the native input checkbook element) |
Yes. I think there would be benefits outside of web components too. I wonder if we'd see libraries rely less on the context API when they could just emit a custom event.
@sebmarkbage Unless you know if any current work here, I'd be happen to stencil something out. |
@nhunzaker Well I think that what is being discussed here would be an alternate strategy. We would not support addEventListener('click', fn). We would instead support element.onclick = fn; |
Yeah I think I'm ok with the idea of |
A problem with properties, attributes, events and children is that you have to know which one to use so you end up with heuristics or explicit namespaces. I don't think explicit namespaces is very ergonomic. Properties are more powerful than attributes because:
So if we only had one, I'd prefer it to be properties. That leaves events. If we use a heuristic, that affects performance negatively since we have to create mappings for it at runtime. It also means that we're claiming a whole namespace. It means that custom elements can't provide an I think that would be unfortunate to claim a whole namespace prefix or type when the precedence of Of course, that will still leave us "children" as special but that's kind of a unique property of React that children of components can be any type of value so we'll have to concede that one constrained special case. |
I took a stab at implementing the model discussed above. @sebmarkbage can you let me know if this matches your thinking? class XCheckbox extends HTMLElement {
connectedCallback() {
this.addEventListener('click', this._onclick);
}
disconnectedCallback() {
this.removeEventListener('click', this._onclick);
}
_onclick(e) {
this.checked = !this.checked;
this.dispatchEvent(new CustomEvent('checkchanged', {
detail: { checked: this.checked }, bubbles: false
}));
}
set oncheckchanged(fn) {
this.removeEventListener('checkchanged', this._oncheckchanged);
this._oncheckchanged = fn;
this.addEventListener('checkchanged', this._oncheckchanged);
}
get oncheckchanged() {
return this._oncheckchanged;
}
set checked(value) {
this._checked = value;
value ? this.setAttribute('checked', '') : this.removeAttribute('checked');
}
get checked() {
return this._checked;
}
}
customElements.define('x-checkbox', XCheckbox);
const props = {
checked: true,
oncheckchanged: function(e) {
console.log('oncheckchanged called with', e);
}
};
const customCheckbox = document.createElement('x-checkbox');
Object.assign(customCheckbox, props);
document.body.appendChild(customCheckbox); One concern is that element authors have to opt-in to defining a setter to expose a handler for every event that they dispatch. That may end up bloating the elements, especially if they have a variety of events that they expose. Having React do |
@sebmarkbage I get why properties are preferable to attributes, that's why Polymer defaults to setting properties. Since most Web Components these days are Polymer elements, and Polymer automatically supports properties, I think most Web Components handle properties correctly. Most of the other WC libraries I've seen handle properties correctly as well. If you know of a large set of components that don't support properties, let me know and I'd be glad to we see if I can help fix that massive loss. @robdodson I don't think it's really feasible to have element authors write their own event handler properties. Events work fine with just Polymer and Angular (and I believe SkateJS as well) have syntax conventions for declaring a binding to a property, attribute or adding an event handler with HTML attribute names. I don't know JSX, but it seems like since it's not HTML and not JavaScript, there's a lot of leeway to invent its own syntax to unambiguously differentiate between properties attributes and events. |
React intentionally went back on that and claims that single event handlers is a better model and the use cases for multiple event listeners is better solved elsewhere since it leads to confusion about where events flow and which order they flow. Additionally, the string based event system is difficult to type. Both statically for type systems like TypeScript and Flow, and for optimizing adding lots of event subscriptions at runtime (instead of string based hash maps). So I don't think it's fair to say that event handler properties are strictly worse. More over, there are other types of first-class event handlers like Observables that would be nice to support using properties. The most important feature for interop is reflection. Without reflection you can't make automatic wrappers such as providing a first-class Observables as properties for each available event. This is something that the event listener system doesn't provide. There is no |
@sebmarkbage those are good points. If you have a moment can you take a look at the sample code I posted and let me know if it seems inline with what you're thinking? |
@robdodson Yes, that looks very good to me. |
If the only problem is boilerplate, I think that is solvable. Initially in user space in terms of libraries that make it easy to create best-practice custom elements. Later a standard helper can be batteries included. |
Yeah that sounds good to me 👍 @treshugart @staltz what do you guys think? |
Looks awesome! I'll reiterate that https://github.com/webcomponents/react-integration currently solves this stuff in userland. That might be a good spot to start collaborating on some of the boilerplate. |
From experience of using vanilla Web Components in a React-like architecture, a few humble suggestions:
|
@robdodson Yeah that's aligned with what I think as well.
I'd also choose the same. What's the action points with this issue? I'm willing to do something but unsure what, and confused whether @robdodson or @sebmarkbage have intentions to act on this. |
To clarify my conclusion: If @robdodson shows that the proposed "best-practice" above is a feasible direction for the Web Components ecosystem including event listeners as properties. Then React will switch to simply transferring props to the element properties, The next actionable item there would be that someone (maybe Polymer?) provides a way to make it easy to build such Web Components and see if it is viable in the more raw-form Web Components community (as opposed to big libraries). |
@staltz I've been discussing it with some of the platform engineers on Chrome. They raised some issues around how the native on* properties work with builtins and I think it'd be worthwhile to discuss those. I'm working on a follow up that details the points they covered. |
This raises the concern: does it make sense to promote a pattern for CEs that uses the existing on* semantics but doesn't have the same attribute / property parallelism? I realize for most frameworks/libraries this is not a problem, because they’ll just use properties. But as a general pattern for CEs it’s troubling because it breaks with how all of the other HTML elements work, and could be surprising to anyone assuming that There are a couple solutions the team proposed:
I personally lean toward option 2, but am curious what other folks on this thread think? Because both of the above options will take some time, that leaves the question of what approach should we move forward with in the near term. I think there are a couple options:
return (
<paper-slider
min={props.min}
max={props.max}
value={props.value}
domEvents={
click: props.onClick
'value-changed': props.onValueChanged
}>
</paper-slider>
Here I lean toward option 1, not that the react-integration lib isn’t awesome, it is(!), but because option 1 lowers the barrier for people to use CEs + React together. In either case, we would continue to encourage folks to treat properties as their source of truth, and we may want to encourage them to only bubble events if they have a good reason to do so. I know that was one of the critiques of the current event system and might be a place where we can agree on a best practice. Both of these options avoid baking in the on* pattern while we hash out what would be a better long term alternative. I’ll add that I’m very motivated to work on this. I think a primary goal of the extensible web movement and Web Components in general is to take feedback from library and framework authors and use that to improve the platform. |
+1 for |
update: after a couple of months of research, and some prototyping, it seems to us that React will have to do nothing new to support web-components interoperability in both ways:
@sebmarkbage we can chat next week at TC39, and eventually share the examples, and the docs if it is sufficient in your opinion. Slotting remains an open question though! |
Well, that's not anything new. As most frameworks, nothing needs to be done to support web components. This issue started out as an improvement suggestion on how to avoid |
As a reminder, could we reconsider
Would be better to have more solid arguments for or against that. |
I agree with @staltz -- we've always been able to interoperate with custom elements inside of React. But it would be really nice if (at least for the majority of custom elements) you could do it without using refs + addEventListener + manual setting of dom element properties, inside of componentWillReceiveProps/componentDidMount |
Wanted to give an update to a proof of concept that helps to solve this problem. I linked to it in #7249 (comment), but here's the gist for convenience. |
I'm keen to see this feature implemented in React. Thanks a lot for your efforts making React great. |
@jfhector I believe the most up-to-date discussions on improved custom element interop for react are now at reactjs/rfcs#15 -- this thread might no longer be relevant. |
FWIW, I published jsx-native-events as a JSX pragma that adds the The following will respond to an event named
/** @jsx nativeEvents */
import React, { useState } from 'react'
import nativeEvents from 'jsx-native-events'
export function SomeComponent () {
const [ name, setName ] = useState('')
render () {
return <div>
<h1>Hello { name }</h1>
<x-custom-input onEventCustomChange={ e => setName(e.detail) }></x-custom-event>
</div>
}
} In case any one is still looking for a stop-gap. |
is there anyway to upvote this so we can have the support out of the box? |
@benkocabasoglu here's a comment from a while ago from Dan Abramov that clarified for me why React has been slow to change/improve React's support for web components. https://dev.to/dan_abramov/comment/6kdc |
That comment isn’t really a great breakdown of the issues. We need an API to get at native DOM events and and API to pass complex data down to a DOM element prop. As far as I can see there is no reason to delay this work even if we just start by designing the JSX syntax to do those things. |
OK, since there hasn't been any engagement on this, I want to take a first shot at syntax for implementing DOM events and properties. For the sake of clarity, I want to define properties which should not be confused with HTML attributes. DOM properties exist on the DOM object (imagine running Obviously for most cases the traditional React way of interacting with the DOM through props would be preferable, I am choosing to use Option 1 — DOM props as objectsThis is fairly straightforward: Treat DOM events and properties as objects similar to how one would pass in <button
domEvents={{
click: someCallback,
mouseenter: e => someOtherCallback(e.x)
}}
domProps={{
disabled: someCondition
}}
>Click me!</button> Option 2 — DOM props as individual React propsThis would likely require the use of a sigil to minimize the risk of colliding with props in the wild. Here we can borrow the syntax from something like lit-html or Angular (I prefer Lit), or we could use a sigil like <button
@click={ someCallback }
@mouseenter={ console.log }
.disabled={ someCondition }
>Click me</button> Option 3 — Prop key exported from React DOMSince this feature would be strictly DOM-based, there could be a key function exported from React DOM that allows interfacing with the DOM events and properties more efficiently. Like the above, this would be necessary to minimize the risk of collisions with keys in the wild. import { domEvent, domProp } from 'react-dom';
// ...
<button
[domEvent('click')]={ someCallback }
[domProp('disabled')]={ someCondition }
>Click me</button> Option three feels clumsy and as far as I know React's JSX doesn't currently allow that kind of computed prop key. Option one feels very React like, but a bit too verbose for my liking. Personally I prefer option two, as it is familiar from users of other view engines and feels React-like enough as not to be a total departure from established norms. |
Option 2 also fits more in line with what other templating engines are doing |
Is there any interest from the React team in implementing a feature like this (never mind one of the options above)? This issue has been open since 2016 and there hasn't been a lot of engagement here. |
There's active work on various ways of approaching this but turns out there are a lot of considerations. For example, how coordination of touch events needs to work for complex gestures and how this interacts with the bubbling, how this works with SSR selective hydration, how you add concepts like scheduling priority to events, how you separate discrete events from continuous events in a concurrent environment, how do you deal with "global" events like keystrokes etc. We've had a few failed attempts and we want to get it right. Now we could add the simple model that just by-passes and leaves those questions unsolved. However, we know that if we just added the simple model, we would want to deprecate it in favor of the current in-progress work. This doesn't seem worth the thrash to us for a minor syntactic improvement. The ecosystem will then have to support both manual refs AND the new syntactic feature while we migrate to the newer system. Instead of just the manual refs that is already a capable option. |
@sebmarkbage Thank you so much for the info, really appreciate the thought and hard work going in to making it the best it can be. While I might be really happy with having to worry about all of those issues, I understand that I'm an outlier in that regard. Is there a place where these discussions are being documented so we can follow along and maybe contribute? |
To use a Web Component in React, you must directly attach an event listener to the React ref for that WC. We could change the implementation so that when React detects a custom element (tag names with dashes, like
my-component
), it will bypass the synthetic event system (and the whitelist) and just attach the event listener on the element itself.Why bypass the synthetic event system? Because anyway we already need to bypass it manually when using a Web Component. I'm not so familiar with the React codebase, but this naive approach seems to work. Whoever uses Web Components in React can be responsible for whatever downsides that would cause, maybe in performance, I don't know. They are already having those (supposed) downsides, this issue is just about the convenience of WC usage inside React.
I was about to send a PR for this, but thought of opening an issue. I looked through the issues and didn't see any existing one related to the handling of WC events.
What is the current behavior?
A WC custom event (e.g.
flipend
) must be handled by attaching the event listener directly to the element in componentDidMount using a ref.http://jsbin.com/yutocopasu/1/edit?js,output
React v15.1.0
What is the expected behavior?
A WC custom event can be handled with
onMyEvent={ev => this.handleMyEvent(ev)}
on the ReactElement corresponding to the WC.PS: this snippet above still has the
ref
, but for unrelated reasons. Ideally we wouldn't need refs for handling events of WCs.The text was updated successfully, but these errors were encountered: