Implement Setting Unknown Attributes #9477

gaearon · 2017-04-21T10:59:30Z

We are currently warning about “unknown properties” in the DOM.
For example if you render:

<div foo="bar" />

you’re going to see a warning from React, and foo won’t actually turn up in the DOM.

People have wanted support for custom attributes since forever: #140. Now that we've actually been warning for a whole release, I think it's a good time to flip this behavior, and to set any unknown attributes on the DOM instead of skipping them.

So the goal is that you would actually see <div foo="bar" /> in your DOM.

There is just one caveat: we still haven't updated all FB callsites to fix this warning. Ideally we want to stay synced with open source version of React, but I don't want React 16 to be delayed because of this, nor do I want delaying this change until React 17. So I think we should bite the bullet, introduce an internal feature flag that will differ for our FB builds, and enable the new behavior in the open source version. Some time during React 16 we’ll finish updating our code, and remove the conditional code path.

I don’t think anybody on the team has time to work on this right now, so I’d love this to be a community contribution. Requirements as I see them:

Introduce a new feature flag to ReactDOMFeatureFlags. Something like shouldSetCustomAttributes. Set it to true.
Keep the warning about unknown DOM props but only enable it if shouldSetCustomAttributes is false. Make sure tests still cover this case (you can override feature flag in tests—see existing tests concerning feature flags for how to do it).
Add new behavior of falling back to setAttribute for any unknown properties (rather than skipping them like we do now). Add tests for it. Those tests shouldn’t need to touch the feature flag (since it’s the new default behavior). Make sure this works both for SVG and HTML.
Make sure Fiber tests pass (when you create a PR, there are instructions on running them).
This might affect server rendering test suite previously added by @aickin. You might need to change those tests to verify the new behavior. It’s fine to only verify the new behavior there (with flag set to true) since we don’t use server rendering ourselves.
Good point from @syranide: Implement Setting Unknown Attributes #9477 (comment). We should still warn for known attributes that are miscapitalized. (It’s fine if that’s a different warning message than the one behind the flag.)
Send the PR!

Please let me know if you’d like to take this. It could turn out a little complicated (there won’t be a lot of guidance from us on this so we probably can’t coach a completely new contributor for this task). But if you sent a PR or two to React, you should be able to do it.

--

Update: @nhunzaker already started a PR on this a while back (#7311) and might be able to rebase it. Let’s discuss the plan more in more specifics below (#9477 (comment)).

The text was updated successfully, but these errors were encountered:

gaearon · 2017-04-21T11:02:09Z

Please make sure to leave a comment here if you want to take it.

gaearon · 2017-04-21T11:05:36Z

cc @aweary in case you were looking for something fun 😄

islautin · 2017-04-21T11:39:52Z

Hey @gaearon I would like to work on it.

Calyhre · 2017-04-21T11:46:20Z

I can give a hand too 🖐

syranide · 2017-04-21T11:50:43Z

What's your take on onClick vs onclick for this?

julien-f · 2017-04-21T11:58:48Z

I think it's a bad idea, it's pretty easy to let some props pass through without wanting too (onclick is a good example).

IMHO, a custom syntax (like a prop prefix) would be much better.

gaearon · 2017-04-21T12:00:21Z

What's your take on onClick vs onclick for this?

I think we should still warn on casing typos. I can hardly imagine this ever being intentional.

jordyvandomselaar · 2017-04-21T12:01:07Z

I think the rendered html should have data-prop=, not prop= to comply to the w3c.

gaearon · 2017-04-21T12:02:53Z

I think it’s important that everyone commenting here also reads #140. There are plenty use cases for non-standard attributes, and the biggest motivation is enabling those + dropping the need for whitelist in production. I’m happy to debate the details of how we approach this, but I don’t want to turn this thread into a bikeshed on whether or not it is useful.

gaearon · 2017-04-21T12:09:18Z

@islautin I don’t mean no disrespect, but I would really prefer that whoever jumps on this has contributed to React before. This is a fairly involved change, and shepherding PRs from new contributors takes a lot of team time (which we currently are running short of). We use this label for issues more appropriate for first-time contributors.

gaearon · 2017-04-21T12:40:28Z

Update: @nhunzaker already had a PR for this in #7311, but it got stale.

@nhunzaker How much time do you think it’ll take for you to circle back to that PR? If you have other outstanding PRs, which do you feel are blocking you from working on it?

nhunzaker · 2017-04-21T12:49:36Z

The scroll-jank PR (#9333) just needs a bit more testing and approval. I was planning to write some DOM fixtures for media elements, but that could wait.

The DOM Factories PR (#8356) is complete work, it just needs to be integrated into the new build system on master and the old build system on 15.6-dev

If we want to get this in as fast as possible, I could definitely switch gears.

cellog · 2017-04-21T13:01:25Z

Misspellings will no longer be caught. Perhaps disabled by default, since this is an edge case for most.

gaearon · 2017-04-21T13:13:25Z

Misspellings will no longer be caught

I don’t see why they wouldn’t (we can still warn for misspelling on things React is aware of, like onClick). Please see discussion above.

gaearon · 2017-04-21T13:14:49Z

The scroll-jank PR (#9333) just needs a bit more testing and approval. I was planning to write some DOM fixtures for media elements, but that could wait.

Perhaps @spicyj could take over it?

The DOM Factories PR (#8356) is complete work, it just needs to be integrated into the new build system on master and the old build system on 15.6-dev

Perhaps @flarnie could take over it?

No pressure to either of you 😄 Just noticed it seems like most of the work is done, and it’s now at approval / cherry-picking stage where it would make for the team to jump on them.

flarnie · 2017-04-21T14:33:42Z

@gaearon & @nhunzaker I can take over integrating the DOM Factories PR with the old/new build systems - that way @nhunzaker will be free to either focus on #9333 or help with this issue. Thanks for asking. :)

gaearon · 2017-04-21T14:34:46Z

Sounds good, thanks @flarnie!

aickin · 2017-04-21T15:57:42Z

This might affect server rendering test suite previously added by @aickin. You might need to change those tests to verify the new behavior. It’s fine to only verify the new behavior there (with flag set to true) since we don’t use server rendering ourselves.

As a pointer to implementors, I think the tests that will be affected are all in the "unknown attributes" suite here: https://github.com/facebook/react/blob/master/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js#L434

I think they are pretty self explanatory, but if not, feel free to ping me!

aickin · 2017-04-21T16:09:11Z

Also, one weird quirk occurs to me: with this patch, I think you still won't be able to add attributes with the names className or htmlFor to the rendered HTML. That's almost certainly fine, but perhaps worth noting.

tchaffee · 2017-04-21T16:11:46Z

We use this label for issues more appropriate for first-time contributors.

@gaearon, the first 5 issues in that list have already been taken by someone and in some cases a PR submitted. That's a lot of reading and digging to come up empty handed. Is there a more efficient way of finding issues that no one has claimed yet?

gaearon · 2017-04-21T16:14:29Z

No—there aren’t that many bugs in React, and we also don’t add a ton of features. So these quickly get taken, and most contribution opportunities are in ecosystem libraries, and related tooling. And of course we appreciate people helping provide support to other people (who make honest mistakes) both in issues and on StackOverflow and chats.

gaearon · 2017-04-21T16:15:52Z

(As a side note I want to slightly change the label system for claimed issues, but that’s a separate discussion. Please file a new issue and I’ll reply to you there.)

tchaffee · 2017-04-21T18:01:40Z

#9482

jordyvandomselaar · 2017-04-21T18:15:03Z

@gaearon I agree that this is really useful, I meant that you could automatically append data- before any custom attributes, so <Element foo="bar"> turns into <Element data-foo="bar"> unless it's an official attribute like href, name, etc ofcourse.

Some widely used libraries which depend on React have used syntax that ends up passing non-standard props to default React components like 'dom' and 'div'. For example: ``` // where this.props contains attributes like 'foo' return <div {...this.props} />; ``` Since we started throwing warnings for this syntax in Reactv15.2 some libraries have not upgraded past v15.2. Now, because of deprecations in 15.5, some libraries will only work with Reactv15.5, and this causes a conflict when other libraries are pinned at v15.1. The overall effect is that either there are version conflicts or the "unknown prop" warning is thrown all over the place, and users of these libraries can't fix the warnings. See facebook#9466 for more context. We are deduping this warning in hopes that it allows more library authors to update to the latest version of React. React v16.0 may take a different approach to this issue. For some related discussion, see facebook#9477 **Test Plan:** - Added a unit test - Manually tested with a fixture that was not committed; https://gist.github.com/flarnie/db011bf54206f30b9983cd4dc674c82e

gaearon · 2017-08-07T17:55:42Z

We have a specific proposal in #10399 and a PR attempting to implement it in #10385.

I’ll close this—let’s follow up there.

gaearon added Type: Enhancement Difficulty: medium labels Apr 21, 2017

gaearon mentioned this issue Apr 21, 2017

Allow custom (nonstandard) attributes. #140

Closed

kitten mentioned this issue Apr 21, 2017

[v2] Minify some parts of the library styled-components/styled-components#681

Closed

flarnie mentioned this issue Apr 22, 2017

Deduplicate "unknown prop" warning to ease pain of version updates #9488

Closed

This was referenced Apr 22, 2017

Get rid of whitelists for domElements and props validation styled-components/styled-components#731

Closed

React 16 Umbrella ☂️ (and 15.5) #8854

Closed

nhunzaker mentioned this issue Apr 26, 2017

Allow custom attributes by default #7311

Closed

nhunzaker mentioned this issue Jul 8, 2017

Let's stop attribute blocking! #10126

Closed

bvaughn mentioned this issue Aug 1, 2017

React 16 RC #10294

Closed

gaearon closed this as completed Aug 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Setting Unknown Attributes #9477

Implement Setting Unknown Attributes #9477

gaearon commented Apr 21, 2017 •

edited

Loading

gaearon commented Apr 21, 2017

gaearon commented Apr 21, 2017

islautin commented Apr 21, 2017

Calyhre commented Apr 21, 2017

syranide commented Apr 21, 2017

julien-f commented Apr 21, 2017

gaearon commented Apr 21, 2017 •

edited

Loading

jordyvandomselaar commented Apr 21, 2017

gaearon commented Apr 21, 2017

gaearon commented Apr 21, 2017

gaearon commented Apr 21, 2017

nhunzaker commented Apr 21, 2017

cellog commented Apr 21, 2017

gaearon commented Apr 21, 2017

gaearon commented Apr 21, 2017

flarnie commented Apr 21, 2017

gaearon commented Apr 21, 2017

aickin commented Apr 21, 2017

aickin commented Apr 21, 2017

tchaffee commented Apr 21, 2017

gaearon commented Apr 21, 2017 •

edited

Loading

gaearon commented Apr 21, 2017

tchaffee commented Apr 21, 2017

jordyvandomselaar commented Apr 21, 2017

gaearon commented Aug 7, 2017

Implement Setting Unknown Attributes #9477

Implement Setting Unknown Attributes #9477

Comments

gaearon commented Apr 21, 2017 • edited Loading

Update: @nhunzaker already started a PR on this a while back (#7311) and might be able to rebase it. Let’s discuss the plan more in more specifics below (#9477 (comment)).

gaearon commented Apr 21, 2017

gaearon commented Apr 21, 2017

islautin commented Apr 21, 2017

Calyhre commented Apr 21, 2017

syranide commented Apr 21, 2017

julien-f commented Apr 21, 2017

gaearon commented Apr 21, 2017 • edited Loading

jordyvandomselaar commented Apr 21, 2017

gaearon commented Apr 21, 2017

gaearon commented Apr 21, 2017

gaearon commented Apr 21, 2017

nhunzaker commented Apr 21, 2017

cellog commented Apr 21, 2017

gaearon commented Apr 21, 2017

gaearon commented Apr 21, 2017

flarnie commented Apr 21, 2017

gaearon commented Apr 21, 2017

aickin commented Apr 21, 2017

aickin commented Apr 21, 2017

tchaffee commented Apr 21, 2017

gaearon commented Apr 21, 2017 • edited Loading

gaearon commented Apr 21, 2017

tchaffee commented Apr 21, 2017

jordyvandomselaar commented Apr 21, 2017

gaearon commented Aug 7, 2017

gaearon commented Apr 21, 2017 •

edited

Loading

gaearon commented Apr 21, 2017 •

edited

Loading

gaearon commented Apr 21, 2017 •

edited

Loading