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

Remove PropTypes from production build (#209) #3818

Merged
merged 3 commits into from
Jan 17, 2018

Conversation

iansu
Copy link
Contributor

@iansu iansu commented Jan 16, 2018

Apply babel-plugin-transform-react-remove-prop-types when building for production.

Closes #209

@gaearon
Copy link
Contributor

gaearon commented Jan 17, 2018

@gaearon
Copy link
Contributor

gaearon commented Jan 17, 2018

Hmm. I feel like that ESLint rule is too restrictive.

It should be perfectly okay to use another component's propTypes inside a propTypes definition.

Button.propTypes = {
  ...Touchable.propTypes,
  color: PropTypes.string
}

The message is also not friendly:

Using another component's propTypes is forbidden 

It should say something like

Reading propTypes from the component is not safe because it may be removed in production

Can you follow up with eslint-plugin-react about this and remove the rule in the meantime please?

@gaearon gaearon merged commit a00cae7 into facebook:next Jan 17, 2018
akstuhl pushed a commit to akstuhl/create-react-app that referenced this pull request Mar 15, 2018
* Remove PropTypes from production build (facebook#209)

* Added react/forbid-foreign-prop-types rule to eslint config

* Removed react/forbid-foreign-prop-types rule from eslint config
@csantos1113
Copy link

I'm too late on this thread. but what about the scenario that @gaearon mentioned about:

Button.propTypes = {
  ...Touchable.propTypes,
  color: PropTypes.string
}

?

@iansu
Copy link
Contributor Author

iansu commented Nov 1, 2018

I made a PR to eslint-config-react to allow for that scenario before we enabled this plugin: jsx-eslint/eslint-plugin-react#1655

@csantos1113
Copy link

@iansu - thanks for the quick reply. Why about this case:

const space = () => {};
space.propTypes = {
    b: PropTypes.string,
    name: PropTypes.string
};

export default class List extends PureComponent {
  static propTypes = {
    ...space.propTypes, // but I'm getting the warning here!
    message: PropTypes.string,
    b: space.propTypes.b // same here
  }

  render() {
     return <div>{this.props.message}</div>
  }
}

@iansu
Copy link
Contributor Author

iansu commented Nov 2, 2018

I'm not sure why you'd be getting the warning in that case. This would need to be addressed in eslint-plugin-react so you should file an issue there.

@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
@iansu iansu deleted the remove-proptypes-production branch October 18, 2019 05:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants