Rewrite in TypeScript #1
Conversation
.travis.yml
Outdated
| @@ -3,3 +3,5 @@ node_js: | |||
| - '8' | |||
| - '6' | |||
| - '4' | |||
| after_script: | |||
| - npm run coveralls | |||
There was a problem hiding this comment.
I don't like coveralls anymore. It's slow and buggy. Could you switch to Codecov? See: sindresorhus/make-dir@9e74675
| @@ -43,7 +48,11 @@ | |||
| "@sindresorhus/is": "^0.2.0" | |||
| }, | |||
| "devDependencies": { | |||
| "@types/node": "^8.0.31", | |||
There was a problem hiding this comment.
Strange that this is not just bundled in TS... It is with Flow.
There was a problem hiding this comment.
I think the reason is that you could use a specific version of the types. If it's bundled inside TS, you can't change it when you use Node 6 for instance.
There was a problem hiding this comment.
Makes sense. I guess I just assumed it would target the Node.js version I was running.
| "prerelease": "npm run build", | ||
| "pretest": "npm run build -- --sourceMap", | ||
| "test": "nyc ava dist/test", | ||
| "build": "del dist && tsc -p tsconfig.json --declaration" | ||
| }, | ||
| "files": [ |
There was a problem hiding this comment.
You need to add the src dir here.
Side question: Why do we use src? I know it's the convention, but source would look much better and clearer. (I've been doing a lot of Swift lately, so influenced by it's clarity).
There was a problem hiding this comment.
You mean dist?
I'm fine with using source as well.
src/lib/predicates/string.ts
Outdated
| * @param number The minimum length of the string. | ||
| */ | ||
| minLength(number: number) { | ||
| this.context.validators.push(value => { |
There was a problem hiding this comment.
Doesn't need to be in this PR, but we should figure out a way so that predicates can just return a value instead of having to push to this.context.validators.
There was a problem hiding this comment.
Yes, would be much better.
tsconfig.json
Outdated
| "pretty": true, | ||
| "allowUnreachableCode": false, | ||
| "allowUnusedLabels": false, | ||
| "noImplicitAny": false, |
There was a problem hiding this comment.
I assume this is because is doesn't have any typings? It will soon: sindresorhus/is#9
There was a problem hiding this comment.
Yes we should set it to true on the long run.
|
This is a great start! Thanks for working on it :) |
Maybe we should make a TSLint XO config? I've been hoping for https://github.com/eslint/typescript-eslint-parser and https://github.com/nzakas/eslint-plugin-typescript to be stable so I can bundle those in XO and have full TS support there built-in. |
I would be so happy if we had this. The thing is that TSLint is a lot less stricter. I was also hoping for the TS ESLint parser. But not sure how it evolves. |
Yeah, let's just ignore it for now. We can add it and clean the code when we make it public. |
| @@ -0,0 +1,3 @@ | |||
| import { ow } from './ow'; | |||
|
|
|||
| export = ow; | |||
There was a problem hiding this comment.
I didn't use export default ow; here because it would require people using CommonJS to use const ow = require('ow').default. The "downside" is that people using TypeScript (not sure about Babel) will have to write import * as ow from 'ow'; instead of import ow from 'ow';.
| } | ||
| }); | ||
|
|
||
| return this; |
There was a problem hiding this comment.
I noticed that in the JS version, you always returned a new instance. Was there a specific reason for this? Or is returning this ok?
There was a problem hiding this comment.
My initial version was just a proof of concept. I did that since I didn't use classes, but what you're doing now looks better.
source/lib/predicates/string.ts
Outdated
| */ | ||
| get alphanumeric() { | ||
| this.context.validators.push(value => { | ||
| if (!/^[a-z\d]+$/gi.test(value)) { |
There was a problem hiding this comment.
Is this regexp correct? I believe the old one was incorrect.
There was a problem hiding this comment.
Yes, the old one was incorrect. I just did something quick to test out predicates. You can remove the g flag in yours though.
|
Yay! This is an amazing start 🌈 |
|
Just curious-what was the reason for rewriting in TypeScript? I'm neither for nor against it, just want to follow along =] |
|
Having all the type definitions auto generated was one of the biggest reasons. That's what makes using Ow so easy, because the entire API is auto completed through intellisense. Also, once you get used to strongly typed JavaScript, you just start missing it when you don't have it. |
I just took a stab at rewriting this in TypeScript. Didn't add TSLint yet and it should be finetuned as well, but I think it's a good start to receive the first feedback.