0.4.0 #8
0.4.0 #8
Conversation
ghost
requested a review
from 
There was a problem hiding this comment.
Looks good. 🙌 I left a few comments that maybe I would recommend reviewing and accepting or dismissing before merging. Otherwise looks like a good release! :)
Before you do release, bump ur own pkg version in your lock file so it's in sync with the release commit.
NOTE: I haven't run the tests locally yet, but noticed a few fixme or todos about some tests not throwing or passing? I can check this PR out locally if there are still issues that you want me to inspect.
CHANGELOG.md
Outdated
| * `RegularExpression` scalar type _generator_ | ||
| * `UnsignedInt` alias for `NonNegativeInt` | ||
| * `UnsignedFloat` alias for `NonNegativeFloat` | ||
| * `yarn.lock` |
There was a problem hiding this comment.
Not sure if it's a good idea to commit and mix both npm & yarn lock files. Have run into issues before were they can get out of sync and cause confusion.
As the package.json file is using npm I would recommend only committing the package-lock.json file for npm as that is what we are using for the build/test/publish scripts. Lock files don't get published, having them here is for someone developing or testing, and the build and tests are using npm.
| ``` | ||
|
|
||
| NOTE: `NonNegativeFloat` and `NonNegativeInt` are also available under the aliases `UnsignedFloat` |
README.md
Outdated
| ``` | ||
|
|
||
| Given this, if we want to create a new type that is essentially the same except for one little | ||
| customizable aspect (e.g., a regular expression type that has ll the same code except the regex is |
There was a problem hiding this comment.
missing an a from has ll the same ...
README.md
Outdated
| parameters. That's the approach we take here. | ||
|
|
||
| Therefore the `RegularExpression` scalar type is really a `GraphQLScalarType` object _generator_ | ||
| that takes two arguments: a name and the regex you want it to use. So to create a new scalar for a |
There was a problem hiding this comment.
Formatting suggestion, use bullet points e.g ->
... that takes two arguments:
- a name,
- the regex you want it to use
| * `regex` - The regex to be used to check against any values for fields with this new type | ||
|
|
||
| ``` | ||
| const MyRegexType = new RegexType('MyRegexType', /^ABC$/); |
There was a problem hiding this comment.
I would recommend thinking about using an object with named params for the function signature instead of 2 params. It provides a simple escape hatch for future modifications to your function signature whilst enabling a simple approach to backwards compatibility.
e.g ->const MyRegexType = new RegexType({ name: 'MyRegexType', regex: /^ABC$/ });
Then again I will admit I am biased 😂
| ); | ||
| }); | ||
|
|
||
| // FIXME: Does nothing. No throw. Call doesn't even seem to get to the parseValue() function. |
There was a problem hiding this comment.
Cleaned up several. Some comments still remain because these are still issues.
RegularExpressionscalar generatorUnsignedIntandUnsignedFloataliasesyarn.lock(with corresponding package updates)