React: 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: https://github.com/facebook/react/issues/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: https://github.com/facebook/react/issues/9477#issuecomment-296170676. 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 (https://github.com/facebook/react/pull/7311) and might be able to rebase it. Let’s discuss the plan more in more specifics below (https://github.com/facebook/react/issues/9477#issuecomment-296179889).

[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 https://github.com/facebook/react/issues/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 https://github.com/facebook/react/pull/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]

The scroll-jank PR (https://github.com/facebook/react/pull/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 (https://github.com/facebook/react/pull/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]

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

[gaearon]

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]

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]

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

Sounds good, thanks @flarnie!

[aickin]

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]

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]

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]

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]

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

https://github.com/facebook/react/issues/9482

[jordyvandomselaar]

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

[gaearon]

We have a specific proposal in https://github.com/facebook/react/issues/10399 and a PR attempting to implement it in https://github.com/facebook/react/pull/10385.

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