chore(typescript): add definitions#96
chore(typescript): add definitions#96remarkablemark merged 3 commits intoremarkablemark:masterfrom nikolay-borzov:typescript-definitions
Conversation
- Make `build` script work on Windows
| "coveralls": "cat coverage/lcov.info | coveralls", | ||
| "lint": "eslint --ignore-path .gitignore .", | ||
| "lint:fix": "npm run lint -- --fix", | ||
| "dtslint": "dtslint types", |
There was a problem hiding this comment.
Not sure it's needed. But I could include it
There was a problem hiding this comment.
Yep, I think including would be useful if the library ever was changed.
There was a problem hiding this comment.
I cannot add dtslint as pre-commit hook. It has no support for passing files. It only accepts a folder
"types/*.*": "npm run dtslint"
Causes an error
There was a problem hiding this comment.
No problem. Running it in CI is enough, thanks for trying!
| parse('<a href="#">Heading</a>', { | ||
| replace: node => { | ||
| if (node.attribs && node.attribs.id === 'header') { | ||
| return { |
There was a problem hiding this comment.
Is this return unnecessary?
There was a problem hiding this comment.
Have no idea. I took this from dom-to-react.js
There was a problem hiding this comment.
I ask because since this returns only when there's an element with id of header, this if-check will get skipped entirely
There was a problem hiding this comment.
This code isn't actually called. It serves only as type checking sample
There was a problem hiding this comment.
I wonder what is type of the returned object though. I haven't figured out it from the source code
There was a problem hiding this comment.
You're right in that the return type is any. I could technically restrict it but I decided to keep it flexible (I'm checking the return value with React.isValidElement()
| @@ -0,0 +1,15 @@ | |||
| { | |||
| "compilerOptions": { | |||
| "module": "esnext", | |||
There was a problem hiding this comment.
Would it be useful to keep the module and lib options as:
{
"module": "commonjs",
"lib": ["es2015"]
}There was a problem hiding this comment.
Ok. But I don't think it matters - tsconfig is only needed for linting types
There was a problem hiding this comment.
Gotcha, I'm ok with leaving it as is then
| "strictNullChecks": true, | ||
| "strictFunctionTypes": true, | ||
| "noEmit": true, | ||
| "jsx": "react", |
There was a problem hiding this comment.
Nit: technically this option is unnecessary since the library does not use JSX
There was a problem hiding this comment.
It's needed for test.tsx since it contains JSX
There was a problem hiding this comment.
Good point, thanks for letting me know!
- Specify return type for `replace`
remarkablemark
left a comment
There was a problem hiding this comment.
Looks good @nikolay-borzov!
|
I'll make a release later tonight, thanks once again for making the pull request @nikolay-borzov |
…efinitions chore(typescript): add definitions
buildscript work on WindowsWhat is the motivation for this pull request?
Add TypeScript definitions
Resolves #95