Implement Setting Unknown Attributes

[gaearon]

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

[gaearon]

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

[gaearon]

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

[islautin]

Hey @gaearon I would like to work on it.

[Calyhre]

I can give a hand too 🖐

[syranide]

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

[julien-f]

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]

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]

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

[gaearon]

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]

@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]

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?