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
fixed PropTypes warnings #358
Conversation
What is keeping this from being merged? |
any updates on this ? |
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.
Thanks for the contribution! There are some issues with dependencies to be fixed as I suggested.
package.json
Outdated
@@ -52,6 +52,7 @@ | |||
"mocha": "^2.2.5", | |||
"mocha-jsdom": "^1.0.0", | |||
"react": "^0.14.0", | |||
"prop-types": "^15.5.0", |
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.
-
Shouldn't it be in
dependencies
instead? -
It's not compatible with
"react": "^0.14.0 || ^15.0.0"
, but only with"react": "^0.14.9 || ^15.3.0"
, so peerDependencies should be modified accordingly. -
Examples are meant to be independent (with their dependencies), so we should add it there as well.
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.
- I think, there is no need to put it to dependencies if react is not there.
- Nice catch! Thanks :)
- Not sure if I understand what you mean by that.
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.
One could not use prop-types
in his project. It should't prevent DevTools to work. React is mandatory because otherwise there will be no monitors.
- Every example has it's own
package.json
and we need dependencies to be present there as well.
package.json
Outdated
@@ -61,7 +61,8 @@ | |||
"webpack": "^1.11.0" | |||
}, | |||
"peerDependencies": { | |||
"react": "^0.14.0 || ^15.0.0", | |||
"react": "^0.14.9 || ^15.3.0", | |||
"prop-types": "^15.5.0", |
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.
I think it would be better to move it in dependencies
as I suggested before and as it's recommended by the author (so one could not use that lib). It should be removed from devDependencies
as well.
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.
Yeah... you are right. Will fix it now.
"react-redux": "^4.0.0 || ^5.0.0", | ||
"redux": "^3.5.2" | ||
}, | ||
"dependencies": { | ||
"prop-types": "^15.5.7", |
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.
If it's already in dependencies
it will be installed anyway, so no need for having it also in peerDependencies
and devDependencies
. I guess the 3rd point was causing this confusion :)
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.
One more commit then )
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.
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... now I got what you mean by 3rd point ) Will fix that as well in a minute
Thanks for the contribution! It's published as |
Removed Warning: Accessing PropTypes via the main React package is deprecated. Use the prop-types package from npm instead.