Support "for", "class", "http-equiv", and "accept-charset" #10169

nhunzaker · 2017-07-13T12:15:23Z

This commit updates the HTML property config and UnknownPropertyHook developer warnings such that is no longer required to specify "className" instead of "class", "htmlFor" instead of "for", and so on.

Both className and class, htmlFor and for, are supported. I do not want this to be a breaking change.

When both are specified within props, React provides the warning:

"className and class were listed as properties on <div />. Both write to the same attribute; use one or the other."

This required updating some tests that were dependent using className vs class for developer warnings.

This feature is implemented separately from #7311 but there should be no functional conflict.

Related issues: #5926, #4331

syranide · 2017-07-13T12:48:53Z

Are we intending to discourage and eventually deprecate the old className and htmlFor then? Just having both for the sake of it doesn't seem like good thing.

gaearon · 2017-07-13T12:50:16Z

Yes, I think we want to transition to just class and for. It’s going to be super churny so IMO it’s fine to support both for a couple of majors. Maybe the warning when you specify both should nudge you to use the newer one.

syranide · 2017-07-13T12:51:59Z

@gaearon 👍 On another note, I'm curious, what's the plan with dropping the whitelist then? From what I understand this has been the plan all along in the minds of some, but this seems like definitively moving towards keeping the whitelist or otherwise there will be churn going back.

This commit updates the HTML property config and UnknownPropertyHook developer warnings such that is no longer required to specify "className" instead of "class", "htmlFor" instead of "for", and so on. Both forms are supported. When both are specified within props, React provides the warning: "className and class were listed as properties on <div />. Both write to the same attribute; use one or the other."

nhunzaker · 2017-07-13T13:02:37Z

Ack. Lint fixed.

nhunzaker · 2017-07-13T13:03:46Z

@gaearon I can update the warning to recommend using the attribute name, but I think we should only recommend it when both are specified. What do you think?

gaearon · 2017-07-13T13:34:48Z

I agree.

gaearon · 2017-07-13T13:36:15Z

this seems like definitively moving towards keeping the whitelist

Could you elaborate on that? I’m not super familiar with this code. From my understanding additions to whitelist are temporary to calm down the warnings, but #7311 would make it possible to later remove them. Do I misunderstand this?

syranide · 2017-07-13T14:14:19Z

@gaearon Since way back there has been a discussion towards doing away with HTMLDOMPropertyConfig entirely, all props would then be passed through as-is. If that is still on the table then it seems logical to understand the outcome of that effort before doing this. Because this PR now follows the attribute naming convention for these props, whereas IMHO it makes more sense to use the property naming convention then (htmlFor). Perhaps that discussion has changed but the devs seemed rather vocal towards going down this path in the past.

syranide · 2017-07-13T14:19:35Z

@gaearon Perhaps I should've read the linked PR more first :) but yeah, if the intention is to follow the attribute naming convention it makes sense to do this. If the idea is to follow the property naming convention it doesn't make sense to do this IMHO.

nhunzaker · 2017-07-13T14:19:48Z

@syranide I'm in favor of following the attribute route, keeping the authoring as similar to static HTML as possible. All attributes that we require the property name for, we convert into the attribute name to write as an attribute anyway.

For me, I would like the roadmap to be:

Allow all attributes we presently require to be specified by their property names. Move users towards the attribute names
With Allow custom attributes by default #7311, allow all custom attributes, providing an option to keep the old behavior for legacy concerns.
Remove the feature flag, allow all attributes "as is". Remove the "className" and "htmlFor" helper warnings.
Trim down the HTML property config as much as we possibly can.
Eliminate all conditions that are roughly "can we write this attribute to the element"
Figure out how to remove the property configs, or drastically minimize them.

nhunzaker · 2017-07-13T14:32:05Z

@gaearon have to shift gears, but I can update that warning later today.

After some discussion in facebook#10169 (comment), we have elected to move forward with using attribute names as the standard way of defining React props on html fields. This commit updates the warning associated with instances where a property _and_ attribute name are given such that it recommends using the attribute.

nhunzaker · 2017-07-13T18:26:28Z

src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js

+      expect(container.firstChild.className).toBe('cleric');
+      expectDev(console.error.calls.count()).toBe(1);
+      expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
+        'Warning: class and className were listed as properties on a <div />. Use "class".\n    in div (at **)',


@gaearon how's this sound? too direct?

Also, I've just added quotes around class and className for consistency.

Chudesnov · 2017-07-14T10:25:22Z

This being on the roadmap raises several questions for me:

Will we someday also switch to onclick, autofocus or even make attributes case-insensitive?
Is the ultimate goal to make JSX in React 100% compatible with HTML?

aweary · 2017-07-14T17:08:02Z

@Chudesnov

Will we someday also switch to onclick, autofocus or even make attributes case-insensitive?

That's a good question, I think it would make sense to adopt one strategy or the other. Having a mix of attribute and property names doesn't seem acceptable IMO. If we implement this change I think it follows that we would eventually implement the same change for all attributes.

Is the ultimate goal to make JSX in React 100% compatible with HTML?

At least with the current spec, JSX can't be 100% compatible with HTML due to other differences, like self-closing tags. But that doesn't mean we can't make it more compatible.

ghost · 2017-07-16T13:18:14Z

Please add support for string styles -

<div style="margin-left: 10px; display: inline-block ....">{text}</div>

This would be last thing which limits jsx from using html inside render function

gaearon · 2017-07-26T13:24:09Z

Some discussion on this: https://twitter.com/dan_abramov/status/890191815567175680

nhunzaker · 2017-07-27T17:15:09Z

Oof. I have a lot of tweets to read.

gaearon · 2017-08-04T13:49:44Z

In the end we discussed against doing this, at least for now.

I previously thought class is only an issue in IE8, but it doesn’t work even with modern features in contexts where you’d expect it to:

const {class, someOtherProp} = this.props; // breaks

If we allowed both, it would put third party components in an awkward situation where they are expected to support both. It gets brittle and confusing.

phiresky · 2017-08-04T13:55:06Z

In the end we discussed against doing this

Noooo

I guess it makes sense, but I looked forward to this.

gaearon · 2017-08-04T14:00:39Z

I wanted to do it, but it doesn’t make as much sense to me now.
We probably should just better articulate why we don’t do it. It wasn’t obvious to me before.

KernelDeimos · 2023-01-29T00:18:04Z

I realize this is like 6 years old but if anyone finds this in the future (probably from the same StackOverflow link) and happens to know better... could you tell me why they didn't do something like this?

function createElement(tag, attributes, ...children) {
    attributes.className = attributes['class'] =
        attributes.className || attributes['class'];
    // ...
}

This seems to dispute @gaearon's claim

[...] would put third party components in an awkward situation where they are expected to support both.

nhunzaker requested review from gaearon and aweary July 13, 2017 12:15

facebook-github-bot added the CLA Signed label Jul 13, 2017

gaearon mentioned this pull request Jul 13, 2017

React 16 Umbrella ☂️ (and 15.5) #8854

Closed

nhunzaker force-pushed the nh-support-class branch from 7400476 to 2795381 Compare July 13, 2017 13:02

nhunzaker commented Jul 13, 2017

View reviewed changes

Add quotations around property names in warning

93088c3

bvaughn mentioned this pull request Aug 1, 2017

React 16 RC #10294

Closed

gaearon closed this Aug 4, 2017

kristianfreeman mentioned this pull request Nov 19, 2019

Fix console warnings cloudflare/workers.cloudflare.com#12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support "for", "class", "http-equiv", and "accept-charset" #10169

Support "for", "class", "http-equiv", and "accept-charset" #10169

nhunzaker commented Jul 13, 2017

syranide commented Jul 13, 2017

gaearon commented Jul 13, 2017

syranide commented Jul 13, 2017 •

edited

Loading

nhunzaker commented Jul 13, 2017

nhunzaker commented Jul 13, 2017

gaearon commented Jul 13, 2017

gaearon commented Jul 13, 2017 •

edited

Loading

syranide commented Jul 13, 2017 •

edited

Loading

syranide commented Jul 13, 2017

nhunzaker commented Jul 13, 2017 •

edited

Loading

nhunzaker commented Jul 13, 2017

nhunzaker Jul 13, 2017 •

edited

Loading

nhunzaker Jul 13, 2017

Chudesnov commented Jul 14, 2017 •

edited

Loading

aweary commented Jul 14, 2017

ghost commented Jul 16, 2017 •

edited by ghost

Loading

gaearon commented Jul 26, 2017

nhunzaker commented Jul 27, 2017

gaearon commented Aug 4, 2017

phiresky commented Aug 4, 2017

gaearon commented Aug 4, 2017

KernelDeimos commented Jan 29, 2023

Support "for", "class", "http-equiv", and "accept-charset" #10169

Support "for", "class", "http-equiv", and "accept-charset" #10169

Conversation

nhunzaker commented Jul 13, 2017

syranide commented Jul 13, 2017

gaearon commented Jul 13, 2017

syranide commented Jul 13, 2017 • edited Loading

nhunzaker commented Jul 13, 2017

nhunzaker commented Jul 13, 2017

gaearon commented Jul 13, 2017

gaearon commented Jul 13, 2017 • edited Loading

syranide commented Jul 13, 2017 • edited Loading

syranide commented Jul 13, 2017

nhunzaker commented Jul 13, 2017 • edited Loading

nhunzaker commented Jul 13, 2017

nhunzaker Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

nhunzaker Jul 13, 2017

Choose a reason for hiding this comment

Chudesnov commented Jul 14, 2017 • edited Loading

aweary commented Jul 14, 2017

ghost commented Jul 16, 2017 • edited by ghost Loading

gaearon commented Jul 26, 2017

nhunzaker commented Jul 27, 2017

gaearon commented Aug 4, 2017

phiresky commented Aug 4, 2017

gaearon commented Aug 4, 2017

KernelDeimos commented Jan 29, 2023

syranide commented Jul 13, 2017 •

edited

Loading

gaearon commented Jul 13, 2017 •

edited

Loading

syranide commented Jul 13, 2017 •

edited

Loading

nhunzaker commented Jul 13, 2017 •

edited

Loading

nhunzaker Jul 13, 2017 •

edited

Loading

Chudesnov commented Jul 14, 2017 •

edited

Loading

ghost commented Jul 16, 2017 •

edited by ghost

Loading