Don't convert attributes for DOM elements to strings for custom elements

[trusktr]

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 onto super.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:

customElements.define('gl-mesh', class extends HTMLElement {
  setAttribute(attr, value) {
    if (value instanceof DOMMatrix) {
        // use raw numbers here, for performance.
    }
    else if (typeof value == 'string') {
        // otherwise, parse a string, which is slower.
    }
    super.setAttribute(attr, value)
  }
})

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 extend setAttribute, 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

[trusktr]

Interesting to note is that attributeChangedCallback can't be used for this, as the HTML engine passes the already-stringified values in at this point.

Also, as further optimization, it is possible that if a non-string is received, not to call super.setAttribute, and an extended getAttribute method can call setAttribute with a string value only at the moment that getAttribute is called, in order to prevent the HTML engine from even doing a string conversion at all. For example, the strings will only be calculated when you open dev tools and inspect the DOM.

But currently this optimization is not possible if React automatically converts all values to strings.

[aweary]

I don't see a problem with removing the string coercion in our setAttribute calls. setAttribute will coerce non-string values anyways. cc @spicyj is there any historical reason we need to explicitly coerce?

[nhunzaker]

This is happening for JSDOM. I think we could get away with not doing it.

[syranide]

@aweary AFAIK it's because setAttribute doesn't always toString the way you would expect. This may have been a problem with just IE8, but I know that it is/was intentional and not just "because".

[trusktr]

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.

[robdodson]

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 https://github.com/facebook/react/pull/8755 will allow this to be done declaratively with JSX. For what it's worth, this works in Preact today. They do a 'property' in element check, and if the property is present, they set it. Otherwise they fall back to using an attribute. Adopting a simple model like that sounds appealing to me.

[robdodson]

some additional interesting points I just heard from a team member:

the parser doesn't call setAttribute when it sets attributes, so it doesn't care that you overrode it. if you clone a node, it'll get cloned attributes without calling setAttribute so anything special you did in setAttribute would only occur for imperative DOM creation

[trusktr]

@robdodson

I think encouraging custom element authors to override setAttribute may not be the best approach as it breaks with platform conventions.

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).

I'd argue instead that if you need to pass rich data to a custom element, use a property.

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 setAttribute. That is basically impossible.

setAttribute is standard, and every framework will use it (React, for example).

I think the work being done in #8755 will allow this to be done declaratively with JSX.

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. setAttribute is more like room-temperature butter.

They do a 'property' in element check, and if the property is present, they set it.

Overriding setAttribute in a Custom Element will simply work everywhere.

They do a 'property' in element check, and if the property is present, they set it.

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 setAttribute should be coerced to string first? I don't think so. If you don't like a pattern, don't use it. But I don't think we should block developer freedom.

Also any "hack" that can bring performance notable improvements may be worth the hack.

[trusktr]

the parser doesn't call setAttribute when it sets attributes, so it doesn't care that you overrode it. if you clone a node, it'll get cloned attributes without calling setAttribute so anything special you did in setAttribute would only occur for imperative DOM creation

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, attributeChangedCallback is not called either, which is parallel with what you just mentioned. In these cases, I manually run attributeChangedCallback with the pre-existing attributes (f.e. in a custom element constructor).

[trusktr]

I believe that browsers should use setAttribute instead of hidden magic, because that would solidify the very notion of extending HTML elements.

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.

[robdodson]

That's sort of like saying extending HTMLElement classes breaks platform conventions.

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.

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 setAttribute. That is basically impossible.

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 :foo.prop syntax for specifically passing properties to elements.

setAttribute is standard, and every framework will use it (React, for example).

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've noticed that when custom elements are created with existing attributes, attributeChangedCallback is not called either

I believe it is. Here's a jsbin example. In the console you should see bar null baz corresponding to the attribute's name, its old value and its new value.

I believe that browsers should use setAttribute instead of hidden magic, because that would solidify the very notion of extending HTML elements.

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.

[trusktr]

@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 setAttribute of A-Frame custom elements.

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 setAttributr.

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.

[gaearon]

Our latest thinking on this is here: https://github.com/facebook/react/issues/11347#issuecomment-339830484. I'll close this as outdated but we'd review a PR that does what's described there.