-
-
Notifications
You must be signed in to change notification settings - Fork 126
Another prop-types lib solution #359
Conversation
| "preact-render-to-string": "^3.6.0", | ||
| "preact-transition-group": "^1.1.0", | ||
| "proptypes": "^0.14.3", | ||
| "prop-types": "^15.5.8", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought: maybe this should be a peer dependency now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you, but the prop-types docs recommends leaving it here ¯_(ツ)_/¯
https://github.com/reactjs/prop-types/#how-to-depend-on-this-package
| if (err) console.error(new Error(err.message || err)); | ||
| } | ||
| } | ||
| PropTypes.checkPropTypes(propTypes, props, 'prop', displayName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - this is how they want us to validate! That makes more sense.
|
@DonnieWest @conceptualitis - which PR do you guys think we should merge? The implementation appears to be roughly the same between the two, mainly tests differing. |
|
I'll merge this for now, but there's also documentation in @DonnieWest's PR we'll want too. |
I goofed up and totally missed that @tkh44 (#352) and @DonnieWest (#351) were already working on this when I started looking at #358.
The big differences here are
prop-typesspits outFooin the test, which had to be done because somewhere in the code the I think FB guards against spitting out the sameconsole.errormessage multiple times. Meaning the variable names have to be different.Let me know what you all think.