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

Handle explicit proptypes and default props #196

Merged

Conversation

ndreckshage
Copy link
Contributor

Great project! This PR combines 2 things my company needed for our codebase. I included as separate commits under a single PR for ease of initial discussion. I'm happy to split it up, or drop parts, based on feedback.

Issue 1: Explicitly defined proptypes (9fc0875)

  • If someone decides to add explicit proptypes for whatever reason, I think it makes sense to prioritize that. Currently the plugin adds a duplicate propType declaration (X.proptypes = {}; X.propTypes = {};), if a component also defines prop types.
  • This is useful if (1) the plugin doesn't handle something, and we want to provide it manually; and (2) for special case prop types, like wrapping with a deprecated prop type log.
  • This is opt in, with mergeExplicitPropTypes

Issue 2: Default props optional (e968b5c)

  • If someone sets defaultProps for a component, it is not a flow error to call that component without passing passing in props manually (flow picks up on the defaultProps). However, this plugin marks the propTypes as isRequired, regardless of defaultProps. This can result in discrepancies based on falsy values.
  • This makes defaultProps optional, and is opt in with defaultPropsOptional.

Issue 3: Anonymous classes in hocs: This was fixed by 20.1.0 as well (thanks!), so I undid my changes, but left in my specs, if helpful.

@coveralls
Copy link

coveralls commented Apr 13, 2018

Coverage Status

Coverage increased (+0.6%) to 92.548% when pulling 3a7147d on ndreckshage:handle-explicit-proptypes-and-default-props into 0ec565f on brigand:master.

@brigand
Copy link
Owner

brigand commented Apr 13, 2018

Wow, this looks great!

It's a bit hard to review the snapshots with multiple tests in one file. Could you please split them up?

mergeExplicitPropTypes

I don't think I want a config option for this. If you're defining a props flow type, and propTypes, merging seems like the correct behavior. If you don't want this, then you can disable the plugin for that file ('no babel-plugin-flow-react-proptypes';).

defaultPropsOptional

Same as mergeExplicitPropTypes, but to a stronger extent of having it enabled being the right behavior, but harder to work around if you didn't want that behavior for some reason. Do you have a specific requirement for this?


This plugin is a small part of your tooling, so adding config options is a high cost to users. Other contributors have also been against adding config options.

@ndreckshage
Copy link
Contributor Author

Great! I'll remove config options, agree. Also, happy to split up tests - do you prefer one snap shot spec per file? Theres a few test files with several specs, so just want to confirm that before creating a bunch of files.

@brigand
Copy link
Owner

brigand commented Apr 13, 2018

Yeah, one snapshot per test file is best.

@ndreckshage
Copy link
Contributor Author

Updated, let me know if any questions / suggestions.

@brigand brigand merged commit 792a27d into brigand:master Apr 20, 2018
@brigand
Copy link
Owner

brigand commented Apr 20, 2018

Sorry for the delay. Published as 24.0.0. Thanks!

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

Successfully merging this pull request may close these issues.

3 participants