Add missing select prop type on State #11
Conversation
While reading the code, I noticed there is a missing prop type. On the `State` component, the `select` prop types was missing.
Codecov Report
@@ Coverage Diff @@
## master #11 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 2 2
Lines 30 30
Branches 7 7
=====================================
Hits 30 30
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Since you are working on this may I ask you to please change the <Provider> propTypes as well?
state is required there
Provider.propTypes = {
inspect: PropTypes.func,
state: PropTypes.object.isRequired
}Thanks a lot for your first contribution.
I really appreciated 🎉🎉
src/state.js
Outdated
| @@ -57,7 +57,8 @@ State.contextTypes = { | |||
| State.propTypes = { | |||
| update: PropTypes.func, | |||
There was a problem hiding this comment.
Could you please remove this?
It's a mistake because update is not supposed to be a <State> prop
There was a problem hiding this comment.
Just to make it clear...it was my fault 😬
|
Thanks for creating this library, I love the idea and how much simpler the code is to read and understand the data flow. Changed both. May I ask why the state is required? Would it not make sense to default to an empty object so that if you don't have an initial state you don't need to specify? |
src/provider.js
Outdated
| @@ -29,7 +29,8 @@ Provider.childContextTypes = { | |||
|
|
|||
| Provider.propTypes = { | |||
| inspect: PropTypes.func, | |||
| state: PropTypes.object | |||
| state: PropTypes.object.isRequired, | |||
| children: PropTypes.node | |||
There was a problem hiding this comment.
children should not be in prop types as we use a render prop instead.
I like it better because it's more explicit than children. It tells exactly what it does
There was a problem hiding this comment.
Ouch I am sorry I thought it was the State component.
Thanks for adding the children here, do you mind adding isRequired for it as well
|
@eXon Thanks a lot. Simplicity and readability were the main points that made me want to share the library.
Technically you are right, but I'd prefer to be more explicit and always encourage the user to use a What do you think? |
|
sorry misclick |
|
Thanks a lot 🙏 |
While reading the code, I noticed there is a missing prop type. On the
Statecomponent, theselectprop types was missing.