Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warning for unknown properties. #6800

Merged
merged 1 commit into from
May 25, 2016
Merged

Conversation

jimfb
Copy link
Contributor

@jimfb jimfb commented May 18, 2016

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

ReactDOM.render(<div foo="bar" />, container);
expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain(
'Warning: Unknown prop foo. See https://fb.me/react-unknown-prop'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sophiebits
Copy link
Collaborator

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
Copy link
Contributor Author

jimfb commented May 19, 2016

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
Copy link

ghost commented May 19, 2016

@jimfb updated the pull request.

@keyz
Copy link
Contributor

keyz commented May 20, 2016

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

@sophiebits
Copy link
Collaborator

getStackAddendumByID is available now.

@jimfb
Copy link
Contributor Author

jimfb commented May 23, 2016

Ok, converted all the warnings to use getStackAddendumByID.

@ghost
Copy link

ghost commented May 23, 2016

@jimfb updated the pull request.

).toBe(
'Warning: Unknown DOM property class. Did you mean className? (**:*)'
console.error.argsForCall[0][0]
).toContain(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

👍 other than inlines.

@ghost
Copy link

ghost commented May 25, 2016

@jimfb updated the pull request.

@jimfb jimfb merged commit de1de9e into facebook:master May 25, 2016
@zpao zpao added this to the 15.y.0 milestone May 25, 2016
@jedwards1211
Copy link
Contributor

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
Copy link
Contributor Author

jimfb commented Aug 29, 2016

@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
Copy link
Contributor

jedwards1211 commented Aug 29, 2016

@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
Copy link
Contributor Author

jimfb commented Aug 29, 2016

@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
Copy link
Contributor

jedwards1211 commented Aug 29, 2016

@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
Copy link
Contributor

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
Copy link
Contributor Author

jimfb commented Aug 29, 2016

@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
Copy link
Contributor

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
Copy link
Contributor Author

jimfb commented Aug 29, 2016

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

@jedwards1211
Copy link
Contributor

@jimfb Then put that in the docs!!!

@gaearon
Copy link
Collaborator

gaearon commented Aug 29, 2016

@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
Copy link
Contributor

jedwards1211 commented Aug 29, 2016

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
Copy link
Collaborator

gaearon commented Aug 29, 2016

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

@jedwards1211
Copy link
Contributor

Under a new subheading called "Best Practices"

@jedwards1211
Copy link
Contributor

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
Copy link
Collaborator

gaearon commented Aug 29, 2016

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
Copy link
Contributor

jedwards1211 commented Aug 29, 2016

@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
Copy link
Collaborator

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
Copy link
Contributor

jedwards1211 commented Aug 29, 2016

@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
Copy link
Collaborator

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
Copy link
Contributor

jedwards1211 commented Aug 29, 2016

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
Copy link
Contributor

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
Copy link
Collaborator

sophiebits commented Aug 29, 2016

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
Copy link
Contributor

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants