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

Allow custom (nonstandard) attributes. #140

Closed
steida opened this Issue Jun 30, 2013 · 131 comments

Comments

Projects
None yet
@steida

steida commented Jun 30, 2013

Various frameworks uses custom attributes. React could allow to extend default data- and aria- prefixes. Something like this:

React.registerCustomAttributePrefix('e-');
React.registerCustomAttributePrefix('ng-');
@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Jun 23, 2014

Contributor

Cross-post from #1730

Have we considered something like an attrs-property? It would pass through any value as-is as an attribute for the DOM (same could be considered for a props-property, although it look kind of ambiguous).

I like it in the sense that it would work like data and aria is going to (soon), and it's use-case is actually very similar (i.e, use unsupported/unofficial attributes) so it kind of makes sense to me.

Contributor

syranide commented Jun 23, 2014

Cross-post from #1730

Have we considered something like an attrs-property? It would pass through any value as-is as an attribute for the DOM (same could be considered for a props-property, although it look kind of ambiguous).

I like it in the sense that it would work like data and aria is going to (soon), and it's use-case is actually very similar (i.e, use unsupported/unofficial attributes) so it kind of makes sense to me.

@Swivelgames

This comment has been minimized.

Show comment
Hide comment
@Swivelgames

Swivelgames Jul 10, 2014

@syranide That's definitely an option to consider, imo (for whatever its worth :P ). It would certainly contribute to making this much less ambiguous or esoteric for new users.

Swivelgames commented Jul 10, 2014

@syranide That's definitely an option to consider, imo (for whatever its worth :P ). It would certainly contribute to making this much less ambiguous or esoteric for new users.

@Swivelgames

This comment has been minimized.

Show comment
Hide comment
@Swivelgames

Swivelgames Jul 10, 2014

@syranide, now, that being said, are you referring to using attrs- for standard properties? Or for React JS properties?

It might be a good idea (especially for users who aren't familiar with ReactJS) to use something like properties suffixed by react- for react-specific attributes. I'm not sure how big of a change we're talking about here, though, and this could be getting into a much larger discussion regarding how React JS essentially works at its core and how it introduces itself to developers, which is certainly not something I'm here trying instigate.

Swivelgames commented Jul 10, 2014

@syranide, now, that being said, are you referring to using attrs- for standard properties? Or for React JS properties?

It might be a good idea (especially for users who aren't familiar with ReactJS) to use something like properties suffixed by react- for react-specific attributes. I'm not sure how big of a change we're talking about here, though, and this could be getting into a much larger discussion regarding how React JS essentially works at its core and how it introduces itself to developers, which is certainly not something I'm here trying instigate.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Jul 10, 2014

Contributor

@Swivelgames Not sure if I understand, my intention with attrs-* (or rather attrs= to align with the coming data= and aria=) is that it would translate directly to attributes on the node. So <span attrs={{abc: 123}} /> would yield <span abc="123" />.

Contributor

syranide commented Jul 10, 2014

@Swivelgames Not sure if I understand, my intention with attrs-* (or rather attrs= to align with the coming data= and aria=) is that it would translate directly to attributes on the node. So <span attrs={{abc: 123}} /> would yield <span abc="123" />.

@Swivelgames

This comment has been minimized.

Show comment
Hide comment
@Swivelgames

Swivelgames Jul 12, 2014

OH, I see what you're saying. I apologize, I misunderstood :)

Swivelgames commented Jul 12, 2014

OH, I see what you're saying. I apologize, I misunderstood :)

@geelen

This comment has been minimized.

Show comment
Hide comment
@geelen

geelen Aug 9, 2014

This is also an issue for interop between React and Custom Elements - something like an attrs property, that allowed passing through arbitrary properties and diffing them as simple strings seems like it would work well here.

geelen commented Aug 9, 2014

This is also an issue for interop between React and Custom Elements - something like an attrs property, that allowed passing through arbitrary properties and diffing them as simple strings seems like it would work well here.

@janhancic

This comment has been minimized.

Show comment
Hide comment
@janhancic

janhancic Nov 10, 2014

Contributor

Do you guys know when this feature is going to land in React?

I'm playing around with node-webkit, which supports a custom attribute nwdirectory on <input type="file"/>s, that allow you to select folders. But I need to find a workaround as React strips it ...

Contributor

janhancic commented Nov 10, 2014

Do you guys know when this feature is going to land in React?

I'm playing around with node-webkit, which supports a custom attribute nwdirectory on <input type="file"/>s, that allow you to select folders. But I need to find a workaround as React strips it ...

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Nov 11, 2014

Contributor

@janhancic this.ref.myInput.getDOMNode().setAttribute('nwdirectory') in componentDidMount is a work-around until there's movement on this.

Contributor

syranide commented Nov 11, 2014

@janhancic this.ref.myInput.getDOMNode().setAttribute('nwdirectory') in componentDidMount is a work-around until there's movement on this.

@janhancic

This comment has been minimized.

Show comment
Hide comment
@janhancic

janhancic Nov 11, 2014

Contributor

Yep, that's what I came up with :) Thanks.

Contributor

janhancic commented Nov 11, 2014

Yep, that's what I came up with :) Thanks.

@Aaronius

This comment has been minimized.

Show comment
Hide comment
@Aaronius

Aaronius Dec 2, 2014

I don't think the workaround @syranide outlined will work in all the necessary cases. For example, I don't believe you can do:

var MyButton = React.createClass({
        displayName: 'MyButton',
        componentDidMount: function() {
            this.getDOMNode().setAttribute('is', 'my-button');
        },
        render: function() {
            return React.createElement('button', $.extend({}, this.props));
        }
    });

And have it be properly upgraded as a "my-button" custom element. I assume because we're setting the is attribute after the element is created and placed in the document. I'd be happy to hear if there's another way to deal with this particular case.

Aaronius commented Dec 2, 2014

I don't think the workaround @syranide outlined will work in all the necessary cases. For example, I don't believe you can do:

var MyButton = React.createClass({
        displayName: 'MyButton',
        componentDidMount: function() {
            this.getDOMNode().setAttribute('is', 'my-button');
        },
        render: function() {
            return React.createElement('button', $.extend({}, this.props));
        }
    });

And have it be properly upgraded as a "my-button" custom element. I assume because we're setting the is attribute after the element is created and placed in the document. I'd be happy to hear if there's another way to deal with this particular case.

@geelen

This comment has been minimized.

Show comment
Hide comment
@geelen

geelen Dec 2, 2014

Good find @Aaronius! I'd love for an official way to do this before the element is added into the DOM

geelen commented Dec 2, 2014

Good find @Aaronius! I'd love for an official way to do this before the element is added into the DOM

@nfroidure

This comment has been minimized.

Show comment
Hide comment
@nfroidure

nfroidure Dec 11, 2014

@syranide thanks for the trick.

As @Aaronius pointed out, it would be great to have something like a componentWillBeAttached(node:DOMNode) method in order to perform any action before its effective insertion in the DOM.

nfroidure commented Dec 11, 2014

@syranide thanks for the trick.

As @Aaronius pointed out, it would be great to have something like a componentWillBeAttached(node:DOMNode) method in order to perform any action before its effective insertion in the DOM.

@nfroidure

This comment has been minimized.

Show comment
Hide comment
@nfroidure

nfroidure Dec 11, 2014

Sadly, the method described above doesn't work server side. A componentWillBeAttached as i mentionned above won't work either server side.

Is it feasible to create a componentWillRender method that would allow to modify outputted HTML either on server and the browser and allow us to simply solve the legacy/custom Tags/Attributes problem with a simple Mixin ?

nfroidure commented Dec 11, 2014

Sadly, the method described above doesn't work server side. A componentWillBeAttached as i mentionned above won't work either server side.

Is it feasible to create a componentWillRender method that would allow to modify outputted HTML either on server and the browser and allow us to simply solve the legacy/custom Tags/Attributes problem with a simple Mixin ?

@nfroidure

This comment has been minimized.

Show comment
Hide comment
@nfroidure

nfroidure Dec 12, 2014

Here is the solution i came accross https://github.com/SimpliField/react/commit/8861a9461c0f4dbac2c6dfb1bfe71a4d8c5fc356.

It allowed me to inject custom attributes for my needs with this piece of code:

var HTMLDOMLegacyPropertyConfig = {
  isCustomAttribute: function(attributeName) {
    return -1 !== [
      'align', 'bgcolor', 'border'
     ].indexOf(attributeName);
  },
  Properties: {
    align: null,
    bgcolor: null,
    border: null
  },
  DOMAttributeNames: {
  },
  DOMPropertyNames: {
  }
};

var React = require('react');

// Allow custom/legacy attributes for mail templates
React.Injection.DOMProperty.injectDOMPropertyConfig(HTMLDOMLegacyPropertyConfig);

Any better way to inject this ? If no, any plan on exposing ReactInjection on the React main object ?

nfroidure commented Dec 12, 2014

Here is the solution i came accross https://github.com/SimpliField/react/commit/8861a9461c0f4dbac2c6dfb1bfe71a4d8c5fc356.

It allowed me to inject custom attributes for my needs with this piece of code:

var HTMLDOMLegacyPropertyConfig = {
  isCustomAttribute: function(attributeName) {
    return -1 !== [
      'align', 'bgcolor', 'border'
     ].indexOf(attributeName);
  },
  Properties: {
    align: null,
    bgcolor: null,
    border: null
  },
  DOMAttributeNames: {
  },
  DOMPropertyNames: {
  }
};

var React = require('react');

// Allow custom/legacy attributes for mail templates
React.Injection.DOMProperty.injectDOMPropertyConfig(HTMLDOMLegacyPropertyConfig);

Any better way to inject this ? If no, any plan on exposing ReactInjection on the React main object ?

@robink

This comment has been minimized.

Show comment
Hide comment
@robink

robink Dec 15, 2014

Just came across the same issue... The workaround mentioned by @syranide won't work on server side. The current situation prevents me from generating email HTML with legacy attributes like "align" "bgcolor".

robink commented Dec 15, 2014

Just came across the same issue... The workaround mentioned by @syranide won't work on server side. The current situation prevents me from generating email HTML with legacy attributes like "align" "bgcolor".

@steida

This comment has been minimized.

Show comment
Hide comment
@steida

steida Dec 15, 2014

On the server side, you can use very dirty string replacement based workaround. Use data-fokfokfok prefix, and it will be safe enough. Still, React should and has to allow custom attributes, because Polymer. @sebmarkbage ?

steida commented Dec 15, 2014

On the server side, you can use very dirty string replacement based workaround. Use data-fokfokfok prefix, and it will be safe enough. Still, React should and has to allow custom attributes, because Polymer. @sebmarkbage ?

@robink

This comment has been minimized.

Show comment
Hide comment
@robink

robink Dec 15, 2014

So you would need to do a workaround for client side and another one for server side. Not very neat... :( Too bad that HTMLDOMPropertyConfig is not easily configurable.

robink commented Dec 15, 2014

So you would need to do a workaround for client side and another one for server side. Not very neat... :( Too bad that HTMLDOMPropertyConfig is not easily configurable.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Dec 15, 2014

Member

We want to move to a model where we render all attributes that you provide. Without a whitelist. There are a few concerns though.

It's a bit dangerous because we might need to change the meaning and signature of those attributes. E.g. as complex properties gets added to HTML, your code might break between versions. We prefer rich data types instead of strings.

We also have the issue of patterns like transferPropsTo used to transfer too many properties and people specified invalid HTML properties. These used to be silently ignored. We would need to provide a nice upgrade path for this case.

We could probably do it for web-components but not HTML or we could commit to always supporting string values and try to find an upgrade path for existing code.

Member

sebmarkbage commented Dec 15, 2014

We want to move to a model where we render all attributes that you provide. Without a whitelist. There are a few concerns though.

It's a bit dangerous because we might need to change the meaning and signature of those attributes. E.g. as complex properties gets added to HTML, your code might break between versions. We prefer rich data types instead of strings.

We also have the issue of patterns like transferPropsTo used to transfer too many properties and people specified invalid HTML properties. These used to be silently ignored. We would need to provide a nice upgrade path for this case.

We could probably do it for web-components but not HTML or we could commit to always supporting string values and try to find an upgrade path for existing code.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Dec 15, 2014

Member

E.g. as complex properties gets added to HTML, your code might break between versions.

But isn't this true in web anyway (i.e. when not using React)?

IMO it's the expected behavior in the web that if HTML adds an attribute and I already use it, whether by accident or on purpose, something might break. Moreover it's not like attributes get added really often.

We also have the issue of patterns like transferPropsTo used to transfer too many properties and people specified invalid HTML properties.

Since transferPropsTo is deprecated, would this still be an issue?

Member

gaearon commented Dec 15, 2014

E.g. as complex properties gets added to HTML, your code might break between versions.

But isn't this true in web anyway (i.e. when not using React)?

IMO it's the expected behavior in the web that if HTML adds an attribute and I already use it, whether by accident or on purpose, something might break. Moreover it's not like attributes get added really often.

We also have the issue of patterns like transferPropsTo used to transfer too many properties and people specified invalid HTML properties.

Since transferPropsTo is deprecated, would this still be an issue?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Dec 15, 2014

Member

@gaearon Yes, but properties are riskier. You're less likely to rely on a property (unless you patch prototypes) than an attribute. We try to model them as properties when possible.

Member

sebmarkbage commented Dec 15, 2014

@gaearon Yes, but properties are riskier. You're less likely to rely on a property (unless you patch prototypes) than an attribute. We try to model them as properties when possible.

@geelen

This comment has been minimized.

Show comment
Hide comment
@geelen

geelen Dec 15, 2014

I didn't follow this last comment @sebmarkbage, are you talking about more complex attribute values in custom elements, that can't be simply represented by strings? I'm not clear on how that would work, I'd have thought that all communication with the custom element needs to be done through the DOM, as strings?

Rather than rendering all attributes without a whitelist, maybe just adding a separate stringAttrs: { attr: val } object that can be used to explicitly declare a list of attributes to be treated as simple strings. That way simple attributes can be supported without breaking code, which is my use case and (from following this issue) seems like most of the others here?

geelen commented Dec 15, 2014

I didn't follow this last comment @sebmarkbage, are you talking about more complex attribute values in custom elements, that can't be simply represented by strings? I'm not clear on how that would work, I'd have thought that all communication with the custom element needs to be done through the DOM, as strings?

Rather than rendering all attributes without a whitelist, maybe just adding a separate stringAttrs: { attr: val } object that can be used to explicitly declare a list of attributes to be treated as simple strings. That way simple attributes can be supported without breaking code, which is my use case and (from following this issue) seems like most of the others here?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Dec 16, 2014

Member

For custom elements, we can only really support simple strings since we don't have any rich information about what they will be. For known HTML elements we can use rich data structures for style, classList, matrices for SVG attributes, etc. Maybe the solution is to simply use setAttribute if a string is provided when we also have rich property support. E.g. style would accept a string. Although that's a security risk so we might not allow that.

The nested stringAttrs property could work, but even then, it might not be great from a security perspective and not very elegant.

I think we can find a way to support all the things. My primary concern is the upgrade path. I have some ideas there.

We could replace existing uses of <div {...this.props} /> with a wrapper that only propagates the whitelist. E.g. <ConstrainedLegacyDiv {...this.props} />

Member

sebmarkbage commented Dec 16, 2014

For custom elements, we can only really support simple strings since we don't have any rich information about what they will be. For known HTML elements we can use rich data structures for style, classList, matrices for SVG attributes, etc. Maybe the solution is to simply use setAttribute if a string is provided when we also have rich property support. E.g. style would accept a string. Although that's a security risk so we might not allow that.

The nested stringAttrs property could work, but even then, it might not be great from a security perspective and not very elegant.

I think we can find a way to support all the things. My primary concern is the upgrade path. I have some ideas there.

We could replace existing uses of <div {...this.props} /> with a wrapper that only propagates the whitelist. E.g. <ConstrainedLegacyDiv {...this.props} />

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Dec 16, 2014

Contributor

@sebmarkbage You are definitely the authority on this, but to me it seems natural to otherwise simply add a dedicated prop for "setting attributes/properties with raw values on DOM nodes" (i.e. attrs={...}) which also comes with the understanding that whatever you provide it the attr gets set to.

It's probably not a very nice idea at all applied to custom elements, but then again, making the default implementation for all DOM elements non-whitelisted is definitely not nice either IMHO, especially as it conflates two very different behaviors without any hint as to which one you'll get.

Obviously, just custom elements could be "pass-through" and it kind of fits nicely with the fact that they too are uniquely named (apart from handful SVG nodes that conflict, unless they need their own namespace anyway?).

Anyway, just rambling here. :)

Contributor

syranide commented Dec 16, 2014

@sebmarkbage You are definitely the authority on this, but to me it seems natural to otherwise simply add a dedicated prop for "setting attributes/properties with raw values on DOM nodes" (i.e. attrs={...}) which also comes with the understanding that whatever you provide it the attr gets set to.

It's probably not a very nice idea at all applied to custom elements, but then again, making the default implementation for all DOM elements non-whitelisted is definitely not nice either IMHO, especially as it conflates two very different behaviors without any hint as to which one you'll get.

Obviously, just custom elements could be "pass-through" and it kind of fits nicely with the fact that they too are uniquely named (apart from handful SVG nodes that conflict, unless they need their own namespace anyway?).

Anyway, just rambling here. :)

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Dec 16, 2014

Member

My rationale is that it is unlikely that a DOM property that accepts a string would have different behavior from the attribute. E.g. .setAttribute('class', str) has the same semantics as .className = str.

Except for the imperative quirks of what time they're mutated. Browsers have bugs or unintuitive behavior for when they update, and at which time you would want to invoke the setter. There's also internal state in the component that might update a property while leaving the attribute the same.

The point is that even if we added the special behavior for className later on, the behavior would be unaffected - except to fix unexpected life-cycle quirks. Having an attrs={...} escape makes it confusing how those differ from the other properties and they also don't get the bug fixes from the other properties.

Member

sebmarkbage commented Dec 16, 2014

My rationale is that it is unlikely that a DOM property that accepts a string would have different behavior from the attribute. E.g. .setAttribute('class', str) has the same semantics as .className = str.

Except for the imperative quirks of what time they're mutated. Browsers have bugs or unintuitive behavior for when they update, and at which time you would want to invoke the setter. There's also internal state in the component that might update a property while leaving the attribute the same.

The point is that even if we added the special behavior for className later on, the behavior would be unaffected - except to fix unexpected life-cycle quirks. Having an attrs={...} escape makes it confusing how those differ from the other properties and they also don't get the bug fixes from the other properties.

@funwithtriangles

This comment has been minimized.

Show comment
Hide comment
@funwithtriangles

funwithtriangles Apr 8, 2017

Would it make sense for a recommended approach to be officially documented somewhere? This thread is quite baffling.

funwithtriangles commented Apr 8, 2017

Would it make sense for a recommended approach to be officially documented somewhere? This thread is quite baffling.

@forrest-akin

This comment has been minimized.

Show comment
Hide comment
@forrest-akin

forrest-akin Apr 8, 2017

I've found this approach to work best in most cases

forrest-akin commented Apr 8, 2017

I've found this approach to work best in most cases

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 21, 2017

Member

I think we’re ready to do it, but I don’t think anybody on the team has time right now. If somebody wants to contribute a big feature to React this is the opportunity 😄

#9477

Member

gaearon commented Apr 21, 2017

I think we’re ready to do it, but I don’t think anybody on the team has time right now. If somebody wants to contribute a big feature to React this is the opportunity 😄

#9477

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Apr 21, 2017

Collaborator

I dug into this a while back:
#7311

I'd be happy to see this through, but I'm chewing on a few other outstanding pull requests right now if someone would like to pick up the work. Otherwise I could circle back this once I close out those pull requests

Collaborator

nhunzaker commented Apr 21, 2017

I dug into this a while back:
#7311

I'd be happy to see this through, but I'm chewing on a few other outstanding pull requests right now if someone would like to pick up the work. Otherwise I could circle back this once I close out those pull requests

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 21, 2017

Member

OK, I think rebasing #7311 is the most reasonable way forward since you already did a bunch of the work. (Sorry I forgot!) I’ll leave a note in #9477 but let’s discuss more specifics there.

#9477 (comment)

Member

gaearon commented Apr 21, 2017

OK, I think rebasing #7311 is the most reasonable way forward since you already did a bunch of the work. (Sorry I forgot!) I’ll leave a note in #9477 but let’s discuss more specifics there.

#9477 (comment)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 8, 2017

Member

After research in #6459, #7311, #10229, #10397, this was fixed by #10385, #10470, #10495, #10564, and some follow up work to ensure correctness in #10536, #10543, #10559, #10588, #10594, #10595.

Learn more about the new behavior in React 16:
https://facebook.github.io/react/blog/2017/09/08/dom-attributes-in-react-16.html

Member

gaearon commented Sep 8, 2017

After research in #6459, #7311, #10229, #10397, this was fixed by #10385, #10470, #10495, #10564, and some follow up work to ensure correctness in #10536, #10543, #10559, #10588, #10594, #10595.

Learn more about the new behavior in React 16:
https://facebook.github.io/react/blog/2017/09/08/dom-attributes-in-react-16.html

@gaearon gaearon closed this Sep 8, 2017

@Swivelgames

This comment has been minimized.

Show comment
Hide comment
@Swivelgames

Swivelgames commented Sep 8, 2017

@gaearon
Thank You

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 8, 2017

Member

Thanks to @nhunzaker who did most of the work. :-)

Member

gaearon commented Sep 8, 2017

Thanks to @nhunzaker who did most of the work. :-)

@everdimension

This comment has been minimized.

Show comment
Hide comment
@everdimension

everdimension Oct 5, 2017

Contributor

Hmmm, but I still need to allow custom attributes on regular dom elements.

I used to be happy with this:

import { DOMProperty } from 'react-dom/lib/ReactInjection';

DOMProperty.injectDOMPropertyConfig({
  isCustomAttribute: attributeName =>
    attributeName.startsWith('cq-') || attributeName.startsWith('stx'),
});

But in react 16 you no longer expose the DOMProperty object although I see you still use it in the source code.

As you write in the release notes:

There is no react/lib/* and react-dom/lib/* anymore < ... > let us know about your specific case in a new issue, and we’ll try to figure out a migration strategy for you

Well, please do! :)

I rely on a library which, in turn, relies on custom elements (web components). I have to provide layout for it.

While I'm not happy that they chose this path of adding custom attributes to the elements, I feel like providing required markup should be accomplishable.

So in short, is there still a way to tell react that certain attributes are okay?

If not, shouldn't this issue be reopened?

Contributor

everdimension commented Oct 5, 2017

Hmmm, but I still need to allow custom attributes on regular dom elements.

I used to be happy with this:

import { DOMProperty } from 'react-dom/lib/ReactInjection';

DOMProperty.injectDOMPropertyConfig({
  isCustomAttribute: attributeName =>
    attributeName.startsWith('cq-') || attributeName.startsWith('stx'),
});

But in react 16 you no longer expose the DOMProperty object although I see you still use it in the source code.

As you write in the release notes:

There is no react/lib/* and react-dom/lib/* anymore < ... > let us know about your specific case in a new issue, and we’ll try to figure out a migration strategy for you

Well, please do! :)

I rely on a library which, in turn, relies on custom elements (web components). I have to provide layout for it.

While I'm not happy that they chose this path of adding custom attributes to the elements, I feel like providing required markup should be accomplishable.

So in short, is there still a way to tell react that certain attributes are okay?

If not, shouldn't this issue be reopened?

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Oct 5, 2017

Member

@everdimension what attributes are you having trouble with? Arbitrary custom attributes should work fine with React 16 by default, which is why this is closed.

Member

aweary commented Oct 5, 2017

@everdimension what attributes are you having trouble with? Arbitrary custom attributes should work fine with React 16 by default, which is why this is closed.

@everdimension

This comment has been minimized.

Show comment
Hide comment
@everdimension

everdimension Oct 6, 2017

Contributor

Ok, I found out what's going on.

The current behavior is something like this:

<span stripped-unknown-attribute>attribute will not be rendered</span>
<span kept-unknown-attribute="">attribute will be rendered</span>

A bit surprising for me, but perhaps intended. In the first case react also gives a warning:

Warning: Received true for non-boolean attribute stripped-unknown-attribute. If this is expected, cast the value to a string.

If this is intended, I'll update my code accordingly.

So is it?... :)

Contributor

everdimension commented Oct 6, 2017

Ok, I found out what's going on.

The current behavior is something like this:

<span stripped-unknown-attribute>attribute will not be rendered</span>
<span kept-unknown-attribute="">attribute will be rendered</span>

A bit surprising for me, but perhaps intended. In the first case react also gives a warning:

Warning: Received true for non-boolean attribute stripped-unknown-attribute. If this is expected, cast the value to a string.

If this is intended, I'll update my code accordingly.

So is it?... :)

@tkrotoff

This comment has been minimized.

Show comment
Hide comment
@tkrotoff

tkrotoff Oct 6, 2017

@everdimension same here, I was surprised that custom boolean attributes are not supported.
Examples of use: AngularJS Material and Angular Flex-Layout.

<div layout="row">
  <div flex hide-lg>First item in row</div>
  <div flex show hide-md="{{hideBox}}">Second item in row</div>
</div>

There are probably other legit use of custom "boolean" attributes. IMHO they should be supported by React.

tkrotoff commented Oct 6, 2017

@everdimension same here, I was surprised that custom boolean attributes are not supported.
Examples of use: AngularJS Material and Angular Flex-Layout.

<div layout="row">
  <div flex hide-lg>First item in row</div>
  <div flex show hide-md="{{hideBox}}">Second item in row</div>
</div>

There are probably other legit use of custom "boolean" attributes. IMHO they should be supported by React.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 6, 2017

Member

They are supported, it just explicitly asks you to pass "true" as a string value.

This is because there is no sure way to tell how to apply a boolean attribute. In some cases it's applied by setting an empty value (for true) and removing it (for false). In other cases it expects "true" and "false" as strings. But again, we can’t always pass them automatically because "false" can be interpreted as truthy (because the string is).

This is why you need to explicitly specify them:

<div something="" />
<div something="true" />
<div something="false" />

depending on how it should be used.

Member

gaearon commented Oct 6, 2017

They are supported, it just explicitly asks you to pass "true" as a string value.

This is because there is no sure way to tell how to apply a boolean attribute. In some cases it's applied by setting an empty value (for true) and removing it (for false). In other cases it expects "true" and "false" as strings. But again, we can’t always pass them automatically because "false" can be interpreted as truthy (because the string is).

This is why you need to explicitly specify them:

<div something="" />
<div something="true" />
<div something="false" />

depending on how it should be used.

@Swivelgames

This comment has been minimized.

Show comment
Hide comment
@Swivelgames

Swivelgames Oct 6, 2017

@gaearon Would it make sense to simply treat the boolean versions as flags but still warn about them? In certain scenarios, just like not passing in custom attributes, supporting libraries and others may rely on the attribute simply existing, not specifically being equal to "true".

In those cases, the following would all be considered "true":

<div something />
<div something="" />
<div something="true" />
<div something="false" />

And the following would be considered "false":

<div />

I especially think with propTypes, FlowType, TypeScript, and others, attributes used as boolean flags are probably done so very explicitly.

This is also true because it's a bit more cumbersome to check for "true", "True", and "TruE" rather than .hasAttribute() and other methods.

Swivelgames commented Oct 6, 2017

@gaearon Would it make sense to simply treat the boolean versions as flags but still warn about them? In certain scenarios, just like not passing in custom attributes, supporting libraries and others may rely on the attribute simply existing, not specifically being equal to "true".

In those cases, the following would all be considered "true":

<div something />
<div something="" />
<div something="true" />
<div something="false" />

And the following would be considered "false":

<div />

I especially think with propTypes, FlowType, TypeScript, and others, attributes used as boolean flags are probably done so very explicitly.

This is also true because it's a bit more cumbersome to check for "true", "True", and "TruE" rather than .hasAttribute() and other methods.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 6, 2017

Member

Then this form works for you, doesn’t it?

<div something="" />

I understand the shorthand form feels “nicer”:

<div something />

But it might actually be confusing in the future. There are discussions in the JSX spec repo about changing the semantics to compile this to something={something}. This would align it with ES6 object shorthand form, and also make it less cumbersome to pass many props down:

const className = 'foo';
const user = props.comment.author;
return <Comment className user />

So I wouldn’t recommend relying on the shorthand form anyway. It’s not changing anytime soon, but within a few years—maybe.

Member

gaearon commented Oct 6, 2017

Then this form works for you, doesn’t it?

<div something="" />

I understand the shorthand form feels “nicer”:

<div something />

But it might actually be confusing in the future. There are discussions in the JSX spec repo about changing the semantics to compile this to something={something}. This would align it with ES6 object shorthand form, and also make it less cumbersome to pass many props down:

const className = 'foo';
const user = props.comment.author;
return <Comment className user />

So I wouldn’t recommend relying on the shorthand form anyway. It’s not changing anytime soon, but within a few years—maybe.

@everdimension

This comment has been minimized.

Show comment
Hide comment
@everdimension

everdimension Oct 6, 2017

Contributor

Using

<div something="" />

works for me.

According to html spec it is semantically the same as using an empty attribute, the latter being an implicit way to give the value of an empty string.

Contributor

everdimension commented Oct 6, 2017

Using

<div something="" />

works for me.

According to html spec it is semantically the same as using an empty attribute, the latter being an implicit way to give the value of an empty string.

@Swivelgames

This comment has been minimized.

Show comment
Hide comment
@Swivelgames

Swivelgames Oct 6, 2017

@everdimension @gaearon Indeed! That's the principal problem here.

The possibility of a false-positive is definitely high in the case of {false} becoming "false", especially since, principally, these values are entirely the opposite of each other. We might as well be setting {false} to "true", because through tests these would yield the same result.

We are, after all, specifically talking about boolean attributes, and the current implementation is not consistent with how boolean attributes should work.

3.2.2 Boolean attributes

A number of attributes in HTML5 are boolean attributes. The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value.

If the attribute is present, its value must either be the empty string or a value that is a case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace.

I argue on the side of intention specifically. Obviously, setting something="false" should absolutely pass through as the same. But boolean attributes are specific types of attributes; <div something="" /> is specifically a valid truthy value.

These are truthy values:

<div something />
<div something="" />
<div something="true" />
<div something="false" />

I think the big "issue" here would moreso just be that the following will result in an undesired "truthy" outcome:

<MyComponent something={false} />

Because instead of rendering it as:

<div />

It would be rendered as:

<div something="false" />
OR
<div something="" />

Creating a sort of "false-positive", when the desired intent would be to prevent the something attribute from being attached by setting it to false. In its current state, the true problem here is not with ensuring a truthy value, but instead with intuitively ensuring a false value. "false" is truthy, and that's the real issue here.

The component example above is explicit, of course, but in practice it could be something={randomlySourcedBoolean}. In the current implementation, the much more verbose and cumbersome solution (especially in the case of large sets of JSX markdown) seems like the only viable one:

if (randomlySourcedBoolean) {
  return <MyComponent something />;
} else {
  return <MyComponent />;
}

Which is not a very intuitive solution :P

Swivelgames commented Oct 6, 2017

@everdimension @gaearon Indeed! That's the principal problem here.

The possibility of a false-positive is definitely high in the case of {false} becoming "false", especially since, principally, these values are entirely the opposite of each other. We might as well be setting {false} to "true", because through tests these would yield the same result.

We are, after all, specifically talking about boolean attributes, and the current implementation is not consistent with how boolean attributes should work.

3.2.2 Boolean attributes

A number of attributes in HTML5 are boolean attributes. The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value.

If the attribute is present, its value must either be the empty string or a value that is a case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace.

I argue on the side of intention specifically. Obviously, setting something="false" should absolutely pass through as the same. But boolean attributes are specific types of attributes; <div something="" /> is specifically a valid truthy value.

These are truthy values:

<div something />
<div something="" />
<div something="true" />
<div something="false" />

I think the big "issue" here would moreso just be that the following will result in an undesired "truthy" outcome:

<MyComponent something={false} />

Because instead of rendering it as:

<div />

It would be rendered as:

<div something="false" />
OR
<div something="" />

Creating a sort of "false-positive", when the desired intent would be to prevent the something attribute from being attached by setting it to false. In its current state, the true problem here is not with ensuring a truthy value, but instead with intuitively ensuring a false value. "false" is truthy, and that's the real issue here.

The component example above is explicit, of course, but in practice it could be something={randomlySourcedBoolean}. In the current implementation, the much more verbose and cumbersome solution (especially in the case of large sets of JSX markdown) seems like the only viable one:

if (randomlySourcedBoolean) {
  return <MyComponent something />;
} else {
  return <MyComponent />;
}

Which is not a very intuitive solution :P

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Oct 6, 2017

Member

@Swivelgames if you do <div something={false} /> you'll get the warning mentioned earlier about a boolean value with an unknown attribute. It will render it as <div />, not <div something="false" /> (example)

To remove a boolean attribute you can use null

<div something={null} />
// renders <div />
<div something="" />
// renders <div something />

Please refer to the previous issues on this topic for more context:

Make on/off, yes/no boolean attributes work #10589
Decide on the desired behavior when a boolean value is passed to an unknown prop. #10521
RFC: Plan for Attributes in React 16
Boolean attributes on Web Components #9230

We understand what the HTML spec says about boolean attributes, and recognize the inconsistency. React is a higher-level abstraction where boolean values can sometimes represent enumerable attributes which have a boolean set of values (spellcheck for example), so it's important that we implement a solution with that context in mind.

Member

aweary commented Oct 6, 2017

@Swivelgames if you do <div something={false} /> you'll get the warning mentioned earlier about a boolean value with an unknown attribute. It will render it as <div />, not <div something="false" /> (example)

To remove a boolean attribute you can use null

<div something={null} />
// renders <div />
<div something="" />
// renders <div something />

Please refer to the previous issues on this topic for more context:

Make on/off, yes/no boolean attributes work #10589
Decide on the desired behavior when a boolean value is passed to an unknown prop. #10521
RFC: Plan for Attributes in React 16
Boolean attributes on Web Components #9230

We understand what the HTML spec says about boolean attributes, and recognize the inconsistency. React is a higher-level abstraction where boolean values can sometimes represent enumerable attributes which have a boolean set of values (spellcheck for example), so it's important that we implement a solution with that context in mind.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 6, 2017

Member

This thread is getting really long and every message spams a lot of people. The feature has been implemented in React 16.

If you have specific concerns or ideas about it please file a new focused issue. Thanks!

Member

gaearon commented Oct 6, 2017

This thread is getting really long and every message spams a lot of people. The feature has been implemented in React 16.

If you have specific concerns or ideas about it please file a new focused issue. Thanks!

@facebook facebook locked and limited conversation to collaborators Oct 6, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.