-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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
Don't convert attributes for DOM elements to strings for custom elements #10070
Comments
Interesting to note is that Also, as further optimization, it is possible that if a non-string is received, not to call But currently this optimization is not possible if React automatically converts all values to strings. |
I don't see a problem with removing the string coercion in our |
This is happening for JSDOM. I think we could get away with not doing it. |
@aweary AFAIK it's because |
After I implement the performance enhance that I described above for my webgl custom elements, I will add a perf comparison here by forking React and disabling the string coercion. I'm not sure when I'll get to it, but I'm keeping track of it. |
I think encouraging custom element authors to override setAttribute may not be the best approach as it breaks with platform conventions. I'd argue instead that if you need to pass rich data to a custom element, use a property. I think the work being done in #8755 will allow this to be done declaratively with JSX. For what it's worth, this works in Preact today. They do a |
some additional interesting points I just heard from a team member:
|
That's sort of like saying extending HTMLElement classes breaks platform conventions. I think our ability to do so will expose interesting use-cases that can bring ideas to move the web forward (possibly this one for example).
That's not feasible, because then it means one would need to write an adapter for every single framework to be able to pass info to properties instead of
That might be true, but only for React. There's like 30 other view libs I can think of. Again, passing anything as a property instead of an attribute isn't yet standard, so it just won't be flexible and easy to make it happen across the huge landscape of frameworks and libraries. Although I like the idea, it's just not as easily wide-spreadable.
Overriding
This is fairly library-specific behavior, not standardized, not something easy to make happen across all frameworks/libraries. In the end, is it really React's call to decide for the app that input to Also any "hack" that can bring performance notable improvements may be worth the hack. |
That's no good! This can be fixed though, and it would be inline with the extensible web manifesto, IMO. I've noticed that when custom elements are created with existing attributes, |
I believe that browsers should use When browsers don't do that (as current), they are defeating the extensible web manifesto, and doing unexplainable magic behind the scenes. It doesn't have to be this way, I believe we can argue for the specs to be updated while maintaining compatibility with old apps. |
Sorry if my phrasing was a little loose there. I guess what I'm getting at is that the Custom Elements spec was designed to let developers extend HTMLElement. I don't know of any similar designs to help folks override built-in pieces like setAttribute. While you can do it, as I pointed out, it will lead to weird edge cases because there has been no design work put into making that idea play nice with the parser.
Most of the frameworks I've encountered already set properties, or provide syntax to let the developer do so. By default Angular will pass properties to a custom element and they provide syntax if you want to specifically set an attribute. As I mentioned Preact also prefers properties (as does Ember I believe), and Vue has
I definitely hear what you're saying. It seems like in practice though, other libraries already prefer setting properties for rich data, or provide syntax to let the developer do so declaratively. While it's not a web standard, it does seem to be a bit of a de facto one.
I believe it is. Here's a jsbin example. In the console you should see
I think you're idea is really cool, don't get me wrong :) My main concern is that given the way the parser works today, overriding setAttribute seems like it could cause some real issues for folks. You're free to do whatever you want with your elements and depending on the context in which you use them you might never hit those corner cases. Maybe if you thoroughly document them then anyone consuming your components would know what to watch out for. But for now at least I'd caution against recommending it as a practice all custom element authors should adopt. |
@ngokevin It would be great to get your thoughts on this. Guys, @ngokevin is one of the authors of the library at http://aframe.io. His aframe-react tool does exactly this trick, passing non-string values into He may not have needed to make aframe-react if React would simply fix this one-liner. Note also that using instance properties to pass non-string data is nowhere near standard like setAttribute is. You might be able to convince a few libraries to use instance properties, but EVERY library already used setAttribute, and hence it is best to allow custom elements authors, like @ngokevin and I, to pass non-strings into Another way to look at is: don't be an API blocker. Let React be an interface to the DOM API, nothing more, and let authors have full decision power on how they use the DOM API beneath React. |
Our latest thinking on this is here: #11347 (comment). |
Do you want to request a feature or report a bug?
Feature
What is the current behavior?
React converts values to strings before passing them to the native
setAttribute
methods of DOM elements.What is the expected behavior?
Custom Elements are landing in browsers. It is possible for Custom Elements to extend
setAttribute
(at least in Chrome) to do custom things with input values before passing ontosuper.setAttribute()
.This means that Custom Element authors can accept values other than strings, which can bring performance benefits. For example, imagine Custom Elements designed for rendering to WebGL, and when using them imperatively, the overhead of strings can be avoided.
For example, suppose we have this class:
If React converts values to strings before ever passing them into
setAttribute
, then Custom Element authors can not allow end users to benefit from performance improvements.Browsers allow anything to be passed into
setAttribute
, so React should do the same. If a Custom Element doesn't extendsetAttribute
, the native super class will do the string conversion anyways, so React doesn't have to.In my case, this would be awesome because then I could pass number-based values to my webgl-rendering Custom Elements, and propagate those directly into WebGL without having to convert strings to numbers every tick (for hundreds if not thousands of WebGL objects).
Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
15.6
The text was updated successfully, but these errors were encountered: