Added NegativeInt and NegativeFloat scalars #6
Conversation
ghost
requested a review
from 
cfnelson
left a comment
cfnelson
left a comment
There was a problem hiding this comment.
Left a few comments here that I think might be important to address before merging. Otherwise looks good 👍
src/NegativeFloat.js
Outdated
| } | ||
|
|
||
| export default new GraphQLScalarType({ | ||
| name: 'PositiveFloat', |
There was a problem hiding this comment.
I think this name was meant to be NegativeFloat ?
src/NegativeInt.js
Outdated
| } | ||
|
|
||
| export default new GraphQLScalarType({ | ||
| name: 'PositiveInt', |
There was a problem hiding this comment.
I believe name is meant to be NegativeInt?
src/NegativeFloat.js
Outdated
| import { Kind } from 'graphql/language'; | ||
|
|
||
| function processValue(value) { | ||
| if (isNaN(value)) { |
There was a problem hiding this comment.
Unsure if this check is for detecting if the value passed is only a valid number or testing for NaN. Making an assumption based on the error message, maybe Number.isFinite(value) would be better suited to validate that the value is not a Number. Assuming that you wish to disallow NaN & Infinity.
Would recommend the changes for the other Number scalar types if the above assumption is correct.
There was a problem hiding this comment.
I'll check isFinite. I may also want to use Number.isNaN(). Thanks.
There was a problem hiding this comment.
There are some other things I can do to maybe make this more robust like checking against Number.MAX_SAFE_INTEGER, etc. for example.
cfnelson
left a comment
cfnelson
left a comment
There was a problem hiding this comment.
Looks good 👍 Will be curious to see how this pkg progresses.
| }, | ||
| "dependencies": { | ||
| "peerDependencies": { | ||
| "graphql": "^0.10.1" |
There was a problem hiding this comment.
@ccuilla You will want to also add graphql as a devDependency in addition to having it as a peerDependency to allow for a simple quick install if someone clones your repo directly and wishes to run tests etc.
…r-scalars Added NegativeInt and NegativeFloat scalars
2 participants
Add a couple of simple scalars that were missed in the first version.