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

Warn for PropTypes that don't exist but do exist with a different case #6153

Closed
sebmarkbage opened this issue Mar 1, 2016 · 9 comments
Closed

Comments

@sebmarkbage
Copy link
Collaborator

const Foo = ({ onAction }) => <div onClick={onAction} />;
Foo.propTypes = {
  onAction: PropTypes.function
};

const Bar = () => <Foo onaction={() => console.log('action')} />; // This should warn

We currently don't warn for properties that don't exist in the propTypes set. I.e. we don't catch typos. This is mostly a legacy artifact and there are many components out there that don't define their full set of props. E.g. when ... spread is used. Few components also define the children prop even though they use it. Warning for all of these might be too much work initially.

However, we can start small. We can find missing properties that have corresponding properties with a different case and suggest that the developer use that instead. This is essentially always a bug.

While it is technically possible to have the same property name with two or more different case, it is considered bad practice to allow both. It causes overhead to test for both in the diffing/rendering. It causes style-guide issues where people have to pick which style to use. It also makes it worth for tooling where search/replace can easily miss less commonly used variants. That's why React only allows a single case, and ideally so should custom components.

@wdhorton
Copy link

wdhorton commented Mar 1, 2016

I'd love to take a crack at this.

@mxstbr
Copy link
Contributor

mxstbr commented Mar 1, 2016

@wdhorton if you don't come around to doing it, I'd love to try too. 👍

@philipmoniaga
Copy link

@wdhorton @mxstbr i'd love to try too, if you don't come

@pranaygp
Copy link

@sebmarkbage I'm interested in helping with this. Should I go ahead and work on this, or is someone already working on a fix for this?

@brigand
Copy link
Contributor

brigand commented Apr 9, 2017

I think this can be closed as propTypes is being removed from core.

@aweary
Copy link
Contributor

aweary commented Apr 9, 2017

Good point @brigand. Any discussion on this feature should be directed to: https://github.com/reactjs/prop-types

@aweary aweary closed this as completed Apr 9, 2017
@gaearon
Copy link
Collaborator

gaearon commented Apr 9, 2017

Feels a bit odd to lose all the discussion though. Shouldn't we at least recreate the issue we possibly care about before closing? Otherwise we're starting from scratch.

@aweary
Copy link
Contributor

aweary commented Apr 9, 2017

@gaearon I've opened new issues at reactjs/prop-types for the ones I've closed.

@gaearon
Copy link
Collaborator

gaearon commented Apr 10, 2017

Thank you!

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

No branches or pull requests

8 participants