Skip to content
This repository was archived by the owner on Dec 16, 2021. It is now read-only.

Conversation

@DonnieWest
Copy link
Contributor

Reactjs just recently released their own PropTypes package as per the documentation here and so I fiddled around and brought it into preact-compat!

Not sure if this is something you'd actually want seeing as:

  • You have your own proptypes package that's working well enough
  • PropTypes are deprecated as of React v15.5

But I was fiddling around and all the tests pass so I figured why not submit as a PR? It's also one less thing to maintain.

If you're open to it though, just let me know if I'm doing something horrible and I'll get this shaped right up 👍

@developit
Copy link
Member

looks like you and @tkh44 thought of this at the same time, haha.
I left a comment on his PR: #352 (comment)

There's something funky going on with the prop-types lib when running in development mode where they've made it hard (impossible?) to validate manually, which is how preact-compat validates (as far as I know). Not sure if you were seeing this as well.

@DonnieWest
Copy link
Contributor Author

I'll take a look at this shortly - I'm running this successfully locally, but there's likely something I missed :-)

@DonnieWest
Copy link
Contributor Author

DonnieWest commented Apr 8, 2017

@developit I might be misunderstanding, but, the odd behavior I'm seeing is the result of the deprecation warning issued by this prop-types package when manually checking proptypes. This is documented in this article and shouldn't affect preact-compat since manual proptype checking only happens in dev and not production

Otherwise, I see tests passing that confirm this is working properly. Your jsfiddle also seems to check proptypes properly as long as you ignore that warning (I see valid resolve to true or false when I have valid and invalid proptypes)

That all being said - it's definitely not ideal to have deprecation warnings for supported behavior and that's enough to make me reconsider merging this. Feel free to merge in if you feel it's worth it, but I can always revisit this when React 16 comes out and the deprecation warning is removed

Edit: I do things a little differently than the other pull request, so you might experience different behavior across the two. Just a note :)

@tkh44
Copy link
Contributor

tkh44 commented Apr 9, 2017

@developit This is way more comprehensive than my PR. Feel free to close #352

@DonnieWest
Copy link
Contributor Author

DonnieWest commented Apr 11, 2017

@developit not sure why tests are failing for components built with createClass() but this seems to be working pretty well for me locally on a project. No more sad deprecation warnings!

I've not tested createClass() locally, so, there's more to look into here. I'll see if I can get to it soon

Edit: Clarification

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants