Closed hellosmithy closed 8 years ago
It works if you set class
instead.
cc @jimfb perhaps? (#3067 is related I assume)
@syranide I think you're right that the #3067 commit introduced the issue
Any custom element will match the isCustomComponent
check and skip all the DOM property operations.
I'm struggling to understand the use-case for #3067. Surely attributes should work the same for custom elements and standard DOM elements?
I'm struggling to understand the use-case for #3067. Surely attributes should work the same for custom elements and standard DOM elements?
I'm not very familiar with all this, but there's a difference between custom tags <customtag>
and web components <web-component>
. Web components are much like React components and aren't themselves visible nodes, so the list of DOM attributes don't inherently apply to them (again, just like it makes little sense passing DOM properties to React components), whereas they do for custom tags which just translate to unstyled DOM nodes.
I assume <custom-tag>
was previously valid too, but as of the introduction of web components that's no longer true? This is my limited understanding of all this anyway, so don't take my word for it!
cc @jimfb
@syranide is correct, there is a difference between DOM nodes and webcomponents (which are much more like React components). For standard DOM nodes, we know a priori that there is no className
attribute, so we can commandeer the namespace for our own purposes. For web components, there is no such guarantee, so we can't steal the namespace. In fact, web component authors may prefer className
over class
for the same reasons we stole it for standard dom components, and to remain compatible, we need to pass it through unmolested.
Another way of thinking about this: For standard DOM nodes, we conceptually have a react-component defined for each node, that happens to take className
, just as we happen to take defaultValue
for uncontrolled form elements and happen to use camel case for attributes. We happen to follow some consistent naming conventions in our framework-provided components. The fact that we do most of our markup transformations at the framework level instead of inside react-component definitions is an implementation detail. Web component authors are free to follow those same conventions (or not), but we need to pass them through without transformations or we'll confuse the web components.
TLDR: Custom elements DO work in React. You must use class
instead of className
because the custom element spec requires that we allow users to specify a className
attribute and we need to preserve that functionality for custom elements.
I agree there's a distinction between custom elements and web components. Custom elements are one of the building blocks of web components but they are a spec in their own right. But there's a use case for creating semantically meaningful custom elements which don't extend default DOM behaviour.
Wouldn't a more consistent approach be to treat all elements the same within React but allow either users to escape attributes somehow or alternatively they have the option to dangerouslySetInnerHTML if they are indeed using web components that need to operate outside the React scope?
@hellosmithy Using a component with a dash is the escape. As per the specification, a node with a dash in it may have arbitrary attributes defined.
@jimfb I'm not sure that's a safe assumption to make. My understanding is that a custom element must have a dash in it. Custom elements are used in web components. But an element with a dash in it is not necessarily a web component.
https://developer.mozilla.org/en-US/docs/Web/Web_Components/Custom_Elements "They are part of Web Components but they can also be used by themselves."
Semantics. Custom element is a subset of the web component specification. We use the term "web component" loosely, but I believe the statement holds if you replace "web component" with "custom element"; namely that the custom element may define arbitrary attributes and process those attributes using arbitrary rules.
@jimfb I think this is a little more than semantics. Either way this change prevents custom elements from being interoperable with core dom elements, when surely there are other options such as escaping special case attributes (className
etc) for cases where people need to and thus not create a breaking change. Unless there is another way to use custom elements as purely custom elements and still use React props?
You'd have to special case/escape a whole ton of attributes including every possible camelcase attribute that could ever (past, present, or future) occur on a native DOM component, not to mention dealing with event handlers and other complexities. It would not be as simple as escaping a couple special-cased attributes; the rules would be complex and confusing (certainly more so than the current attribute whitelisting) - I can't see a way to do such a thing in a way that's better than what we currently do (pass attributes straight through).
I'm suggesting there should be a discussion on the best way to do it, escaping may not be it.
One way would be an escape pattern convention e.g. myPropEscaped
(just a straw man example). But I'm also wondering, if you want to create web components that are outside of React's scope surely that is a case for using dangerouslySetInnerHTML
, I would have thought that the default behaviour should be interoperability otherwise there's no way to apply React attributes such as className
to custom elements, which seems an odd and inconsistent restriction. On the other hand there are other ways to pass through web component attributes in those cases where you might want to, and in those cases surely you should be explicitly saying "I don't want React to do the default behaviour in this instance", rather than it just automatically making that assumption for you.
Looking at the JSX code:
<div className="myClass" />
<div is="custom-elem" className="myClass" />
<custom-elem className="myClass" />
It seems right to expect those to behave consistently.
@jimfb @zpao has there been any more thought on this?
Having read the thread at https://github.com/facebook/react/issues/140 I can see why this direction was taken, but I think it raises inconsistencies and will make code harder to reason about. I really do think escaping is the more consistent and flexible option, and forces code to be explicit if it's breaking React convention. Would be interested to hear what your thoughts are.
@hellosmithy I can't see how to make it work in a way that's better than what we currently have. If you have an escaping solution that you'd like to propose, we'd be happy to take a look.
The constraints are:
class
and className
(zero, one, or both). Users must also be able to specify any combination of onclick
and onClick
(zero, one, or both). This is because custom elements can impose arbitrary processing rules on their attributes, and case sensitivity matters.@jimfb @sebmarkbage I'm happy to submit a PR for escaping attrs for web components. However this seems like a separate issue. The changes that have gone through in this release candidate make it difficult to reason about how React will treat different DOM elements in JSX. The onus should be on those wanting to use web components with their own attribute API's to do so in a way that operates safely with existing React behaviour including the way it treats custom elements.
The changes also seem to contradict what is said here by @sebmarkbage: https://github.com/facebook/react/issues/5052#issuecomment-145594782
"The primary component strategy for React will never be Web Components."
So it seems odd to me that a breaking change is being introduced here to appease those that want to use web components inside their React components. While I understand the original sentiment, this feels like the wrong solution is being rushed through, and I worry that once it's gone through into a release it will be 10x harder to argue for it to come out again.
This breaking change has to get a big thumbs down from me. Custom elements are useful today. Web Components are not and probably never will be. It's frustrating to lose the semantic goodness of custom elements for no practical reason that helps developers today.
+1 for Custom Elements support
The problem here is actually the determination of what a developer intends by using a custom component. Here you're assuming that a custom html tag will become a web-component and you decide non intuitive attribute behaviour. In a lot of cases custom html elements are used to provide meaning to the HTML based components. The fix here would be to have the option of setting the the behaviour. Here - ReactDOMComponent.js#L637-L641 - You're testing for a custom component and then deciding that it means a need for custom attribute handling. This is wrong and should be in the hands of developers not framework assumptions. it could easily be set in the component before render is called if this component is intended for promoting as a web-component. This way you remove the need for a breaking change and at the same time provide the functionality you want on a developer chosen basis.
To be clear: Custom elements DO work in React. You must use the class
attribute instead of className
because the custom element spec requires that we allow users to specify arbitrary attributes (including className
) and we need to preserve that functionality for custom elements.
There is no conflict with Sebastian's comment in https://github.com/facebook/react/issues/5052#issuecomment-145594782. Web components are NOT our primary use case, but we will still do the best we can to support the DOM technologies for anyone who wants to use those technologies. Same goes for custom elements. This is why we've added support for them, while consistently recommending you use React composite components instead.
If someone would like to propose a solution that is better than the one we've implemented, subject to the constraints I mentioned above, we'd be more than happy to take a look.
To be clear: Custom elements DO work in React.
@jimfb they may work in the sense that they can be rendered to the DOM, but they will not work with React attributes. Even if we put the inconsistency of using class
sometimes and className
other times aside for a moment, there is a bigger issue here. None of the React events will work such as onClick
, onDrag
etc. Effectively you are leaving "React land" by using a custom element.
I think @mikeybox hit the nail on the head when he said this should be in the hands of developers not the framework.
I'd like to propose an <AttributeEscape/>
wrapper component. I'd advocate this solution for 5 reasons.
So how would this work? It would intercept any child elements, create new DOM elements, assign the props unchanged and dangerously set inner HTML.
You could also create a white/black list feature giving the developer total control over how attributes are handled.
The beauty of this solution is that React has already solved this problem. It can be a third party solution and does not need to be part of the react library.
Here's a couple of examples
// 1
<my-image-component className="someClass" src="someSrc" />
All attributes live in WebComponent land
// 2
<AttributeEscape>
<my-image-component className="someClass" src="someSrc" />
</AttributeEscape>
In this example, we actually want className
to behave as normal.
// 2.1
<AttributeEscape ignore={['className']}>
<my-image-component className="someClass" src="someSrc" />
</AttributeEscape>
Conversely, you could have an include prop, which would trigger AttributeEscape wrapper to ignore all props by default, and only escape the ones we want to include.
// 2.2
<AttributeEscape include={[’src']}>
<my-image-component className="someClass" src="someSrc" />
</AttributeEscape>
The important idea to take away is: The Web Components solution should not be part of React. It should be a third party solution.
I haven't personally used Web Components/Custom Elements alongside React. However, reading @jimfb:
Web components are NOT our primary use case
To me, this makes it even more obvious that React should NOT be handling this itself and should instead leave it to developers / third party (something like @gargantuan suggests).
If it is not the primary use case for React, as a developer, if I know that, and I still put myself in a situation where I want to use both at the same time, then I am prepared to go through a few hoops.
I think it would make sense for React to stay as is, and let developers decide what they want to do.
@jmfb:
the custom element spec requires that we allow users to specify arbitrary attributes
I think we all agree that this is correct. A developer should be allowed to pass all parameters and that is fine. The key here is allow and not enforce. In its current state you're not able to choose and that is the problem. Custom HTML elements do not require this behaviour to be used and essentially most developers will not want to lose basic React element behaviour when using a custom element. By it's very nature custom elements can cover a vast range of intended uses.
The simple conclusion is that yes it is right to enable this raw attribute mode. But it should most definitely be a developer choice to opt in to the functionality. This can be either a new component type or a Class property setting.
@gargantuan That does not allow custom elements to play as first class citizens. You've effectively turned React into a templating system at that point because your AttributeEscape component destroys the virtual dom. You are eliminating React's ability to do diffing (because you convert the virtual dom to dangerouslySetInnerHTML), eliminate the ability for the parent components to make decisions based on the children, eliminate the ability to attach refs and event handlers, etc.
I think I've not clearly explained how this one example proposal would work. I stress that this shouldn't be part of React. You would only put a web-component inside an AttributeEscape component.
That said, I think it's wrong to focus on my half baked proposal and to ignore the wider problem that others have raised.
In addition, my proposal has two saving graces:
Finally - I would plead with you to at least deprecate custom elements and issue a warning before releasing this breaking change. I think that would give the wider community the chance to feed back on change that I'm sure will surprise them.
@gargantuan as per my comment above, it means that elements within AttributeEscape will not play by the vdom rules. This DOES break lots of things. And it's a huge problem if your render tree has an AttributeEscape with an arbitrary tree of children placed inside.
@jimfb the solution suggested by @gargantuan is one option, with it's own merits and considerations. It probably deserves it's own PR and discussion thread.
The point being made here by me and several other commenters is that the RC1 changes related to custom elements also have their pros and cons and should also be justified to the same stringent standards you are requesting for alternative solutions. Several of us have raised concerns related to consistency, backwards compatibility, explicitness, and developer control. Specifically there are concerns about attributes such as className
and all of the React events such as onClick
, onDrag
etc, which will behave in unexpected and non-declarative ways with the proposed RC1 changes.
I think the point being made here is that the proposed changes at https://github.com/jimfb/react/commit/b1db817dc54eb37e05e5dc1b8586efa8e93b0dac require some further consideration.
Given that "the primary component strategy for React will never be Web Components."...
If it were up to me (which I appreciate it's not), I would apply the following constraints to the proposed changes:
className
and React events.Now I appreciate that there has been a vocal request for the ability to support web components, and specifically to allow attributes to be passed through to them. However I would appeal to your better judgement to consider, is this change really the right solution?
Lets not forget, we are not suggesting to add a feature here, but in fact to hold off from adding in a breaking change without due diligence.
I think the primary motivating factor for allowing something is simply to make it easier once you hit an edge case where you need to consume a custom element and wrap it somehow.
We have escape hatches that you're not supposed to need but still might need for practical reasons.
On Oct 7, 2015, at 3:42 PM, Ben Smith notifications@github.com wrote:
@jimfb the solution suggested by @gargantuan is one option, with it's own merits and considerations.
The point being made here by me and several other commenters is that the RC1 changes related to custom elements also have their pros and cons and should also be justified to the same stringent standards you are requesting for alternative solutions. Several of us have raised concerns related to consistency, backwards compatibility, explicitness, and developer control. Specifically there are concerns about attributes such as className and all of the React events such as onClick, onDrag etc, which will behave in unexpected and non-declarative ways with the proposed RC1 changes.
I think the point being made here is that the proposed changes at jimfb@b1db817 require some further consideration.
Given that "the primary component strategy for React will never be Web Components."...
If it were up to me, I would apply the following constraints:
Must not break existing React behaviour with regards to events and React attributes such as className and React events. Users must be able to explicitly specify when they want a DOM elements attributes to be controlled outside of React. Should be explicit and declarative Now I appreciate that there has been a vocal request for the ability to support web components, and specifically to allow attributes to be passed through to them. However I would appeal to your better judgement to consider, is this change really the right solution?
Lets not forget, we are not suggesting to add a feature here, but in fact to hold off form hastily adding in a change without due diligence.
— Reply to this email directly or view it on GitHub.
I think the primary motivating factor for allowing something is simply to make it easier once you hit an edge case where you need to consume a custom element and wrap it somehow.
@sebmarkbage I appreciate there are motivating factors, but are the counter points being fully considered here? Consistency, backwards compatibility etc. Declarative code in particular is one of the things that makes React so great. I realise in this instance the issues raised here were not necessarily apparent when the change was made but there are some pretty strong arguments here for seeking a better solution now that they have been raised.
The discussion here is not be about wether any of the proposed alternative solutions is the right one, but rather wether any of the solutions including the one in RC1 is the right one.
@hellosmithy If you have a better solution, please do propose it. But please consider that we really did spend an enormous amount of time discussing options&alternatives; this was not a spur-of-the-moment change. We can't hold back all development for an indeterminate amount of time. At some point, we need to bite the bullet and make changes in order to make React better. Ultimately it's just code, and there isn't anything that can't be undone if we find something better in the future, but we did do our due diligence in this case.
If you have a solution that you think is better, let us know. We implemented the best thing we've seen to date, but we're always open to new ideas.
@jimfb but the best solution you are referring to breaks React attributes and events on custom elements. Are you saying this was considered and deemed acceptable?
Using native attributes (instead of the react flavors) for custom elements is the best solution we've seen. Better event support is still planned, but was blocked by a couple of browser bugs that prevent us from supporting them reliably. Event support is tracked by several other issues, most notably https://github.com/facebook/react/issues/4751.
Yes. We need a way to render the nodes themselves somehow so that nested React children can work correctly.
It is not possible to map props properly. The only thing that we can safely map automatically is attributes. The rest have to be applied imperatively. So we could either allow no props or at least do attributes automatically.
It turns out that a lot of custom elements work with attributes alone. So it seems like a decent trade off.
If you don't use custom elements then you're not affected.
Why wont you deprecate the current behaviour?
In our case, we have a React component that renders a custom element that behaves like a checkbox. The custom element comes from a library that is used throughout our company so we're just wrapping it so we can use it more easily in React (mainly exposing certain properties on the React component for setting handlers for custom events that the custom elements trigger). Internally, the custom element uses a native checkbox for various reasons...one being that if it's within a form and the form is submitted that the value gets sent with the form. Primarily, the custom element just applies a nice custom skin.
I would like all of React's "special properties" (checked, defaultChecked, etc.) that React implements for a native checkbox to apply to my React component as well. I've poked around ReactDOMInput to see how I might apply it to my component but it doesn't really look like it's possible...or at least it isn't clear to me how to do so. Anyway, if there were some way to do that it would be tremendously helpful. Otherwise, we end up trying to manually reproduce these same properties and their behaviors on our React components.
Any tips on how to do so would be helpful. Even a "sorry, you just have to manually recreate those properties on your components" would be helpful as well. Thanks!
Yea, you just have to manually recreate those properties on your components. Sorry. There has been some talk about making these special behaviors composites instead of built-ins but that doesn't seem likely short term.
With a custom element you can probably do better and avoid some of the weird special cases that browsers have to deal with.
The need for providing direct attribute mapping to a custom element has caused issues for a number of React users. I think the main one being simply that custom elements are in themselves not special. This is the view taken in the w3 specification, they're not special and shouldn't be treated as such. The current way of handling custom elements is to change react behaviour and thereby making them a special case which is often counter productive. I realised today while coding that React already has a precedence for dealing with mapping data straight to DOM. This is seen in the style and dangerouslySet attributes. It would then seem to me that the best course would be to again use that way of thinking to provide a dangerouslySetAttributes (it's probably not dangerous its just user managed) attribute that can be provided with an object to be mapped to the element if required.
for instance
let customAttrs = {
hasChildren: true,
childName: 'Thomas The Tank Engine'
}
<item-preview dangerouslySetAttributes{ customAttrs }/>
Result:
<item-preview data-reactid=".0.0" haschildren="true" childname="Thomas The Tank Engine"></item-preview>
I think this would provide the means of passing arbitrary attributes to the element easy but more importantly assumptions wouldn't need to be made as to the purpose of the custom element.
I've seen that @syranide has already made a similar suggestion here https://github.com/facebook/react/issues/5090. I'm also not keen on the children being set as a serialised version of the child elements. I think it's wrong to treat props and attributes the same and that appears to be where a lot of the confusion is happening with custom-elements. React now handles these elements by dumping the props on to attributes but in most cases this is probably unwanted behaviour. Allowing custom attributes and enforcing custom attributes are two different things and you're doing the latter.
After working without custom-elements for a few weeks now, I think it's worth pointing out that my initial fears around losing custom-elements have materialised in force. Inspecting the DOM is now a lot more difficult than it was before. It is now riddled with generic elements that make it difficult to scan the hierarchy of a view composed of several components, which themselves contain components. Attempts to mitigate this with custom attributes, e.g. <div data-role="my-component">...
makes things worse as it's just more noise. Using the className attribute has the same problem.
Custom elements were a brilliant way to improve the readability of the DOM. Considering React users will spend a large amount of time inspecting the DOM with developer tools; a sane, readable DOM hierarchy has to be considered an important developer friendly feature. That it's been removed for the benefit of the Web Component crowd is incredibly frustrating.
@gargantuan I'm having trouble understanding what you mean, sorry. FWIW, we recommend you use the React dev tools when debugging React apps because they show the React component structure. If they don't fit your workflow for whatever reason, maybe we can fix that instead.
@spicyj - there are a few issues with the React dev tools
In practice I'm still doing most of my debugging in the Elements view and the console. The React tool typically means lot of context switching, and now there are no custom-elements, it means the elements views bears no relation to the react view. As a result, I've found that I rarely use the React Dev tools.
I think that focusing on the React Dev tools to fix the shortcomings introduced by making custom-elements second class citizens would be over engineering.
I think that focusing on the React Dev tools to fix the shortcomings introduced by making custom-elements second class citizens would be over engineering.
Now I'm really confused as to what you mean. Second class citizens in React or second class in the browser? All React does is render the elements, with all the attributes, as they're passed in. What happens after that point, in the inspector view, is outside React's control.
The issues you listed above sounds like general React devtools issues, which should probably be tracked in https://github.com/facebook/react-devtools
Thanks for the answer to my question, @sebmarkbage.
@jimfb - Any React attribute (e.g. className) applied to a custom element is now ignored by React. That's what I mean when I say custom-elements are second class citizens. It's no longer possible to write idiomatic JSX with custom elements.
This was a breaking change that came out of the blue for me and others and did require a sizeable refactor for anyone who had been using custom-elements. Refactors are not a bad thing - but the reasons for using custom-elements remain while their usefulness has been taken away. No one enjoys a refactor that means their codebase becomes more cluttered and difficult to read.
On React Dev tools: They are neither the issue nor the solution to the issue. I have no complaints about React Dev tools because there are ways for me to do my work without them - but they are not the right tool for the way I work.
This issue is, and always has been - Custom elements were useful for non-Web Component users and now they're not. It's clear that you don't see the merits of custom elements, and It's clear you don't think anything has been lost with this change. But If you're not going to acknowledge the merits other people see, then there is no discussion to be had we will just have to lump it.
@gargantuan I'm still having trouble understanding exactly what your specific complaint is here. I'm not an expert in web components so please forgive me if this should be obvious. Is the issue only that you could previously write
<my-x className="foo" />
and now must write
<my-x class="foo" />
instead? Am I misunderstanding completely or just missing something else that has changed?
I can assure you that the changes made to how custom elements work in 0.14 were intended to make them more useful, not less.
@spicyj it's not just className
although that is the original issue that was raised. Any DOM element props such as all event handlers (onDrag
, onClick
etc to name a couple) will also no longer work. It seems strange to have these DOM elements treated differently, and especially to do so automatically without the developer explicitly opting out of React handling the props. I actually really like @mikeybox's suggestion of some kind of setAttributes
or dangerouslySetAttributes
attr which would be much more explicit and transparent to the developer.
class
vs className
is a bit of a hindrance and an inconsistency. But the event handling is particularly bad I feel. I'd much rather a consistent interface for dealing with DOM elements, including custom elements and be able to add events within React e.g adding the same kind of normalized event handling for onDrag
or onClick
rather than have React make the decision for me to switch over to completely escaping the props and passing everything through as web component attributes.
If I created some kind of custom image type as a web component for example I might want it's "web component" api to be src
but I'd still want to control things like drag and click events from within React. In that instance I feel it's much clearer what's going on if it's assumed that attrs are props unless explicitly declared otherwise. The user would explicitly opt out of React controlled props rather than React implicitly making that decision based on something arbitrary such as wether the element has a hyphen in it's name.
E.g.
let escapedAttrs = { src: 'http://...' };
return <custom-image onDrag={ this.handleDrag } setAttrs={ escapedAttrs } />;
As I mentioned earlier in this thread...
Better event support is still planned, but was blocked by a couple of browser bugs that prevent us from supporting them reliably. Event support is tracked by several other issues, most notably #4751.
We want to support events on custom elements - that is on our todo list - but there are some nuances regarding custom-element events that need to be considered and handled properly, and some browser bugs/inconsistencies that were making that infeasible. The current workaround (until events on custom elements can be officially supported by React) is to attach a ref
and add the event handlers imperatively.
gargantuan, regarding your prior post, you said:
@jimfb... It's clear that you don't see the merits of custom elements...
Please keep these comments in check; you weren't in the room when these topics were being discussed, and you couldn't be more wrong on this particular point. If you were at our developer meetings, you would see that I've been the strongest proponent of custom elements. You would see that I've been pushing incredibly hard to provide custom element support. You would see that I'm always pushing to keep React as compliant with the new web standards as is humanly possible.
We make every attempt to listen to all the feedback and continue to improve React to meet everyone's needs, but you need to understand that custom elements are still considered an experimental technology and the details are much more nuanced than you might expect. When browsers start to converge on the semantics, we can re-introduce some of the diffs that fix the rough edges.
@jimfb in response to the events support stuff - maybe I'm misunderstanding but that looks like a potential change to the way events work in general. Wether that happens or not I still don't understand why it wouldn't be preferable to have a consistent approach right now for both events and className
or any other props that may get added in the future for all DOM elements - then allow developers to escape attrs if they want. Has this been considered and decided it wasn't viable?
@hellosmithy Yes, it was considered. That approach does not meet constraint numbers 2 and 5 from https://github.com/facebook/react/issues/4933#issuecomment-143836508
The
className
prop does not appear to get mapped correctly when applied to custom DOM elements in0.14.0-rc1
.JSFiddle: https://jsfiddle.net/hellosmithy/5pdujnfq/1/