Add warning for unknown properties.

[jimfb]

Adds a warning for unknown properties. This unblocks #140, but is probably an intelligent thing to do anyway (regardless of what we do on that issue).

We had previously punted on adding this warning because it was difficult to act upon without source information. We now have source information attached to the elements, so we can provide context with the warning, which now makes the solution tractable, so we can now add this warning.

cc @gaearon @spicyj

[gaearon]

Let’s make the warning more descriptive, similar in style to the other warnings being tested in this file?

[sophiebits]

We should show the full stack. @keyanzhang is going to add a ReactComponentTreeDevtool.getStackAddendum(debugID) in #6799 and then we can use it here.

[jimfb]

Yeah, I'm fine with adding a call to getStackAddendum. In general, I don't like holding up diffs on other unmerged diffs when unnecessary, but it looks like that diff might land imminently, so it's fine. I'll update this PR after that merges.

[ghost]

@jimfb updated the pull request.

[keyz]

@jimfb Are you blocked by #6799? If yes then I can push ReactComponentTreeDevtool.getStackAddendumByID separately first. 😃

[sophiebits]

getStackAddendumByID is available now.

[jimfb]

Ok, converted all the warnings to use getStackAddendumByID.

[ghost]

@jimfb updated the pull request.

[sophiebits]

Can you please keep these all as toBe and check the full error message so we can be confident the message looks right and that it doesn't change accidentally? There are other examples in tests where we do this with stacks.

[sophiebits]

👍 other than inlines.

[ghost]

@jimfb updated the pull request.

[jedwards1211]

Using delete would be such a big PITA (I have literally hundreds of components to change) that I decided to just add a prop filter to all of my spread props. But my prop filter function should really be in React's public API because this a major change for developers like me.

<div role="alert" {...stdProps(this.props)} className={className}>

const reactProps = {
  children: true,
  dangerouslySetInnerHTML: true,
  key: true,
  ref: true,
  ...
}

const domProps = {
  "accept": true,
  "acceptCharset": true,
  "accessKey": true,
  "action": true,
  "allowFullScreen": true,
  "allowTransparency": true,
  ...
}

const eventProps = {
  "onAbort": true,
  "onAbortCapture": true,
  "onAnimationEnd": true,
  "onAnimationEndCapture": true,
  "onAnimationIteration": true,
  "onAnimationIterationCapture": true,
  "onAnimationStart": true,
  "onAnimationStartCapture": true,
  "onBlur": true,
  ...
}

const allProps = {...reactProps, ...domProps, ...eventProps}

export function stdProps(props) {
  const result = {}
  for (let prop in props) {
    if (allProps[prop] || prop.startsWith('data') || prop.startsWith('aria')) result[prop] = props[prop]
  }
  return result
}

[jimfb]

@jedwards1211 If we just got rid of the whitelist, then components which are currently blindly forwarding props (ie. all the props that we warn for), would end up spewing a ton of unnecessary/unexpected markup into the DOM. The warning is there because people have large existing codebases, and we need to give them an opportunity to clean up their prop forwarding before we send it through to the DOM in React v16.

For further reading:
https://gist.github.com/jimfb/d99e0678e9da715ccf6454961ef04d1b#gistcomment-1816271

[jedwards1211]

@jimfb did I simply miss something, or why haven't there been warnings in the documentation for a long time not pass random props to DOM elements? I always thought it would be okay because it seemed to be consistent with how you can pass random props to custom components and they'll just be ignored. But clearly you guys knew this was coming for a long time.

[jimfb]

@jedwards1211 We couldn't add this warning until we could get the component stack trace (because the warning would be too difficult to find/fix), and we couldn't get the stack trace until we added the internal devtools API. So yes, there was a chain of dependencies blocking this.

[jedwards1211]

@jimfb I'm talking about in the documentation, not the code. The documentation should have advised against this even before the code could warn.

[jedwards1211]

So if there are other planned changes that may bite us in the future, please put them in a future changelog in the documentation as soon as possible so that we can write more future-proof code.

[jimfb]

@jedwards1211 If you want to be futureproof, I would recommend not passing additional props to child components (including custom components) that are not expecting those props. We have talked about prohibiting that (for custom components) in the past. One of the main reasons we don't currently prohibit it is that we haven't had the time/energy to implement such a warning, and it hasn't been a priority for us (maybe we never will ban it, but we do tend to think it is a bad practice).

But anyway... the "future changelog" is github issues. That's where you'll always find our latest thinking on any given topic, and the list of things we plan to change at some point in the future. If you watch for pull requests, that's where you'll find things that are imminent. We do our development out in the open for exactly this reason.

[jedwards1211]

Would that prohibition be in React.createElement or only be enforced when the element actually gets mounted? Because for example I do the following:

<Form>
  <Input formGroup label="Name" value={name} />
</Form>

formGroup and label are not used by Input. But Form looks at those and renders:

<div>
  <FormGroup label="Name">
    <Input value={name} />
  </FormGroup>
</div>

[jimfb]

@jedwards1211 You should never create an element with unexpected props.

[jedwards1211]

@jimfb Then put that in the docs!!!

[gaearon]

@jedwards1211 Would you like to help out with this? Since you experienced the confusion, you should have a good idea of which docs should be clarified. Maybe you could send a PR? Thanks!

[jedwards1211]

Sure, is there any particular place you would like that? There's a long tips section but it hasn't really evolved into a fully-organized best practices yet.

[gaearon]

I’m not sure myself. Where would you expect it to be?

[jedwards1211]

Under a new subheading called "Best Practices"

[jedwards1211]

And if there's anything else you expect to be prohibited in the future, please let me know now so I can add it to the PR. Because not all of us have time to scour the github issues to see what's going to break our code in the future

[gaearon]

Warnings are not breakage. They are the mechanism we use to warn people about future breakage. That they’re spammy in console is a problem but we can solve this in the future, e.g. see #7360.

Rather than create a new top-level entry, I think it’s worth revisiting the parts of the docs where we introduce the idea of props, as well as JSX spread, and adding some notes there.

[jedwards1211]

@gaearon I know, but I am frustrated because it seems like you guys were planning this change for potentially even years before the warnings landed. I'm thinking, if only the docs had explained why I shouldn't pass inapplicable props from the get-to, then I would never have had to go back and rewrite the prop passing of so many components. Does that make sense?

Put another way, isn't this one of the things a roadmap is for?

[sophiebits]

We were not planning this for years. When we decide that it's wise for people to convert their code to a different style, that's when we make warnings, lint rules, and blog posts about it.

[jedwards1211]

@spicyj you guys were talking about wanting to get rid of the whitelist at least a year ago in #140. Did you really not know back then that inapplicable props would eventually be forbidden?

[sophiebits]

We didn't reach agreement on it. Even today we don't have total agreement that we want to do this, but we did end up agreeing that it is at least bad style to pass down extra attributes, so we added the warning now. We don't always have perfect foresight.

[jedwards1211]

Sigh. It turns out the docs already do refer to passing unknown props as an anti-pattern. I guess I did read that at some point. I had foolishly decided to start passing everything because of good old performance anxiety...I figured if I could pass down all the props instead of creating a new object, it would perform better and wouldn't hurt anything since the unknown props weren't getting used.

[jedwards1211]

Here's a question. Do you guys consider it bad practice to create an element that's missing some of its required properties but will be cloned to add the missing properties by a wrapper component?

The fact that createElement returns an object that can be passed around and modified via cloning in myriad possible ways gives rise to all kinds of temptation, like how I used unknown props on children of <Form> to control how they get cloned/wrapped. So maybe I could add some advice to the cloneElement documentation about things like this.

[sophiebits]

Type checking (including PropTypes and Flow) generally assumes that you don't do this, but if that's not an issue for you (or you mark the props optional) then the pattern is otherwise supported.

[jedwards1211]

So much black magic is possible with cloneElement that I worry you may eventually deprecate it in the future in favor of patterns like the way react-motion passes the injected props to its child function.

[sophiebits]

If/when we decide we want to deprecate that, we'll announce it. We don't have any plans now to do so.