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

Feature request: warn when property not in propTypes is passed into component #1587

Closed
jameslong opened this issue May 22, 2014 · 30 comments
Closed

Comments

@jameslong
Copy link

It would be great if in debug mode react would warn you when you pass in a property on props which is not specified in the propTypes object for that component. i.e. a way to enforce that props should ONLY have the props specified in propTypes and no others.

This would prevent use of undocumented properties and also allow better code maintenance as unused props would be more easily removed.

@syranide
Copy link
Contributor

To add to this, why does isRequired only emit a console warning? I assume it has to do with not wanting to use invariant for dev-only (same behavior dev and prod), but it's a little weird.

@zpao
Copy link
Member

zpao commented May 22, 2014

We want this too :) @sebmarkbage has some plans here. One idea recently was this: https://github.com/reactjs/react-future/blob/master/01%20-%20Core/08%20-%20Transferring%20Props.js

It's a bit tricky because of transferPropsTo (or whatever we do instead) where you might have props that need to be passed on even if you don't consume them.

Eg <FancyButton className="foo" color="blue" /> which renders this.transferPropsTo(<button value={this.props.text} style={something with this.props.color} />). You want the className to make it through even though you didn't explicitly say that FancyButton uses that.

@zpao
Copy link
Member

zpao commented May 22, 2014

@syranide that's a tangential discussion but ultimately we skip propType checks in prod (they are slow to perform on every update). So we can't use invariant and have different runtime behavior in dev vs prod.

@sophiebits
Copy link
Collaborator

I'm not 100% convinced that we can't use invariant() for prop types in dev only, especially given that the implementation here of warning() is not very aggressive.

@syranide
Copy link
Contributor

@zpao @spicyj Tangential discussion I agree, but just to state my opinion on this. I agree that different behavior in dev and prod is bad... but to me it's about warnings being very easily missed, when something isRequired and isn't provided, it is an error, please tell me! I don't see it any differently than asserts, which are usually not shipped in prod, but stops execution if they fail in dev. An error that exists only in dev isn't really an issue, it's helpful. An error that exists only in prod is a huge red flag.

@slorber
Copy link
Contributor

slorber commented Jul 11, 2014

@jameslong another idea could also be to have a "strict" config in the JSX compiler that will refuse to compile JSX files if they use components without passing all the declared propTypes.

It would only work for JSX components however, but will not affect React at runtime in DEV or PROD...

That way it's like guaranteeing that a component is used in a typesafe way by its clients.

I don't know if something like this would be easy to do, I suppose it could be complicated with transferPropsTo and things like that...

@syranide
Copy link
Contributor

@slorber Since the set of props aren't necessarily very static at all it can't be resolved at compile-time, especially not if you consider that it must also verify the type of each prop.

@slorber
Copy link
Contributor

slorber commented Jul 11, 2014

Yes at compilation time, it can at most be a best effort to try to find obvious component misusages but will not really be typesafe...

The props are not static but if the parent has a string props and it's passed to the child, at least this can be verified that the same type is passed from the parent to the child, and could issue a warning is the parent proptype is "any" while the child has "string" because this "casting" may be unsafe

@syranide
Copy link
Contributor

@slorber If you can't guarantee something at compile-time then you still have to pay the full cost at run-time (for DEV). I agree though that there may be some potential in doing the most basic checks of static types at compile-time, but I'm also doubtful that it would really be worth it... if you didn't even bother testing it once in DEV (which would throw an error in your face) before going PROD then you have bigger issues to deal with on your end.

@WickyNilliams
Copy link
Contributor

I'd just like to chime in to say I'd love this feature.

I always start out with best intentions, documenting all the props. But as I develop further, propTypes inevitably get forgotten about. In my mind propTypes are a contract/interface and any non-documented props should be treated as a violation.

Even if an error is preferable, a warning (as with mismatch) would be better than the what we currently have.

@mczepiel
Copy link

Was about to create a duplicate of this issue myself; it would be appreciated to help keep track of the state of things when I'm in the midst of adding a new feature or refactoring.

@slorber
Copy link
Contributor

slorber commented Nov 24, 2014

yes it would be nice and somehow goes in the direction of Facebook Flow?

@antris
Copy link

antris commented Jan 13, 2015

+1 for this.

@aaronabramov
Copy link
Member

+1 for this feature.
It would be nice to have it if you want to enforce props definition/documentation in the project.

i wonder if it's possible to explicitly whitelist generic components that will accept more props than they have in their propTypes.

based on the example that @zpao gave in #3514

FancyButton = React.createClass {
  propTypes: {
    color: React.PropTypes.string
  },
  render: function() {
    return <button {...this.props} style={color: this.props.color} />;
  }
}
React.render(<FancyButton color="blue" value="foo" />, ...)

@gajus
Copy link
Contributor

gajus commented Aug 30, 2015

I have developed a higher order component that does this.

https://github.com/gajus/react-strict-prop-types

You can control whether HTML properties are allowed using allowHTMLProps option. It uses a list of all standard HTML attributes, including DOM properties such as className and htmlFor and handles dynamic properties such as data-*.

@slorber
Copy link
Contributor

slorber commented Sep 1, 2015

Nice @gajus

Was concerned about the performance cost but yes deactivating this in production mode would be nice!

@gajus
Copy link
Contributor

gajus commented Sep 1, 2015

Yes, you are right. You can easily disable it in the production, https://github.com/gajus/react-strict-prop-types#production-mode.

@tj
Copy link

tj commented Jul 17, 2016

Please don't! Not everyone uses/likes prop types, or at least make it opt-in somehow :D.

@aweary
Copy link
Contributor

aweary commented Jul 17, 2016

@tj I would assume any implementation would only warn if propTypes is defined on the component. So if you don't use propTypes at all you would be fine.

@tj
Copy link

tj commented Jul 17, 2016

True true. I though this was already in 15.x but I was just doing <varName instead of <VarName which leads to warnings on the props

@gaearon
Copy link
Collaborator

gaearon commented Jul 17, 2016

Please don't! Not everyone uses/likes prop types, or at least make it opt-in somehow :D.

Don’t worry, we don’t like them that much either 😄
Longer term we’ll be encouraging people to use Flow (or TypeScript) instead.

@tj
Copy link

tj commented Jul 17, 2016

Awesome, I still have to try Flow, definitely would prefer to just use language features, even if they're still transpiled. I haven't been screwed too bad ignoring them so far haha

@sodik82
Copy link

sodik82 commented Oct 29, 2016

Longer term we’ll be encouraging people to use Flow (or TypeScript) instead.

@gaearon is there any guide that explains how to use Flow instead of proptypes especially how to check/catch unknown prop types (as per this issue). thanks.

@AlicanC
Copy link
Contributor

AlicanC commented Oct 29, 2016

Install flow-bin and initialize the config:

cd your/project
yarn add --dev flow-bin
yarn run flow init

Use it:

/* @flow */

import React from 'react';

type Props = {|
  foo: string,
  bar: any,
|};

type State = {|
  baz: number,
  nullable: ?string,
|};

export default class MyComponent extends React.Component<void, Props, State> {
  props: Props;
  state: State = {
    baz: 13,
    nullable: null,
  };

  componentWillMount() {
    this.setState({
      baz: 'bleh', // Fails, you told "baz" would be a number in State type
      other: true, // Fails, you told there will only be "baz" and "nullable" in State type
    });
  }

  componentWillReceiveProps(nextProps: Props) {
    this.props.baz; // Fails, you told there will only be "foo" and "bar" in Props type
    nextProps.baz; // Fails for the same reason

    const mustPassString = (str: string) => console.log(str);

    mustPassString(this.state.nullable); // Fails, "nullable" can be string, null or undefined

    if (typeof this.state.nullable === 'string') {
      mustPassString(this.state.nullable); // Cool!
    }
  }
}

const a = <MyComponent bar={() => null} />; // Fails, needs "foo"
const b = <MyComponent foo="Test" bar={() => null} baz={13} />; // Fails, "baz" shouldn't be here
const c = <MyComponent foo="Test" bar={() => null} />; // Cool!

You can use Nuclide to see errors in Atom:

git clone [email protected]:facebook/nuclide.git
cd nuclide
npm install
apm link

@mqklin
Copy link

mqklin commented Jan 9, 2017

Flow is nice, but it can't check at runtime. So the only option for me is to use react-strict-prop-types decorator.

@mqklin
Copy link

mqklin commented Jan 10, 2017

react-strict-prop-types doesn't support recursion (eg for PropTypes.shape) :(

@mqklin
Copy link

mqklin commented Feb 21, 2017

Solution: https://github.com/airbnb/prop-types (forbidExtraProps)

@gajus
Copy link
Contributor

gajus commented Feb 21, 2017

react-strict-prop-types doesn't support recursion (eg for PropTypes.shape) :(

PR welcome.

@aweary
Copy link
Contributor

aweary commented Apr 9, 2017

PropTypes have been removed from React core and now exist as a separate package prop-types. Any future feature requests, bug reports, or changes should be directed to the new repo at https://github.com/reactjs/prop-types.

@aweary
Copy link
Contributor

aweary commented Apr 9, 2017

I've opened a new issue for this at facebook/prop-types#11 for anyone looking to continue this discussion.

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

No branches or pull requests