Type definitions#48
Conversation
This comment has been minimized.
This comment has been minimized.
ChristianMurphy
left a comment
ChristianMurphy
left a comment
There was a problem hiding this comment.
Thanks @uetchy!
Could you add dtslint type tests? (see remarkjs/remark-rehype#13, or any of the other typing PRs for an example)
| "dependencies": {}, | ||
| "devDependencies": {}, | ||
| "peerDependencies": { | ||
| "unified": "^9.0.0" |
There was a problem hiding this comment.
remark/unified generally doesn't use peerDependencies, they can cause trouble.
Generally dependencies or devDependencies are used.
There was a problem hiding this comment.
ok. I'll move it to dependencies.
There was a problem hiding this comment.
Sorry, I want to reopen this!
While I think including a package for types is fine (e.g., types/hast, vfile). I don’t think we should do it for unified. I think it should be removed from dependencies, and a note added in the readme that they should be installed (although: plugins don’t make sense w/o unified, so they probably always are installed).
|
@ChristianMurphy I think all issues you mentioned are resolved now. |
ChristianMurphy
requested review from
a team,
BarryThePenguin,
Rokt33r and
remcohaszing
ChristianMurphy
added
☂️ area/types
ChristianMurphy
left a comment
ChristianMurphy
left a comment
There was a problem hiding this comment.
Thanks @uetchy !
Would it be possible to add some simple type tests for dtslint?
Something simple like:
unified().use(rehypeKatex);
unified().use(rehypeKatex, {output: 'html'});
// $ExpectError
unified().use(rehypeKatex, {noteARealOption: true});for each typing
Generally that is used when settings are generic. |
@ChristianMurphy Added type test for each package! |
To fix `npm test`
Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
| "dependencies": {}, | ||
| "devDependencies": {}, | ||
| "peerDependencies": { | ||
| "unified": "^9.0.0" |
There was a problem hiding this comment.
Sorry, I want to reopen this!
While I think including a package for types is fine (e.g., types/hast, vfile). I don’t think we should do it for unified. I think it should be removed from dependencies, and a note added in the readme that they should be installed (although: plugins don’t make sense w/o unified, so they probably always are installed).
add eslint-disable line to suppress dtslint error ref: remarkjs#48 (comment)
|
Any news? |
wooorm
left a comment
wooorm
left a comment
There was a problem hiding this comment.
The changes generally look good, but I’m not a fan of the changes to the ordering of package.json props. Could you revert those?
add eslint-disable line to suppress dtslint error ref: remarkjs#48 (comment)
Co-authored-by: Titus <tituswormer@gmail.com>
|
Thanks @uetchy, released!!! ✨ |
Closes remarkjsGH-48. Reviewed-by: Christian Murphy <christian.murphy.42@gmail.com> Reviewed-by: Titus Wormer <tituswormer@gmail.com>
5 participants
Summary
Fixes #47
Added
.d.tsfile to each package.Discussion
optionshave index signature[index: string]: unknownasunified.Settingsdid?{position: boolean}{position: boolean}to the definition files..use({settings: {position: false}})to the end of the chain.