Conversation
fivdi
left a comment
fivdi
left a comment
There was a problem hiding this comment.
Sorry, I reviewed this PR months ago but it looks like if forgot to submit the review.
| declare module 'lcd' { | ||
| import { EventEmitter } from 'events'; | ||
| export default class Lcd extends EventEmitter { | ||
| constructor(args: { |
There was a problem hiding this comment.
Can args be renamed to config so that the naming is the same as the corresponding JavaScript code?
| rs: number, | ||
| e: number, | ||
| data: [number, number, number, number], | ||
| cols: number, |
There was a problem hiding this comment.
cols is optional and defaults to 16.
| e: number, | ||
| data: [number, number, number, number], | ||
| cols: number, | ||
| rows: number, |
There was a problem hiding this comment.
rows is optional and defaults to 1.
| data: [number, number, number, number], | ||
| cols: number, | ||
| rows: number, | ||
| }); |
There was a problem hiding this comment.
The constructor supports an optional largeFont option but this option missing in args above.
| "mutexify": "^1.2.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/node": "^11.12.1", |
There was a problem hiding this comment.
What purpose does @types/node serve as a dependency. I realize that EventEmitter can be found in @types/node but I don't understand why the @types/node devDependency is needed. At what point will @types/node be used?
There was a problem hiding this comment.
If you are using node core libraries in a Typescript project you need @types/node otherwise nothing will build. But given in this case it is required as a dev dependency, its inclusion is entirely harmless, since it is only being used during development and testing*. End-users can avoid installing dev dependencies by installing a production build.
Really an npm module with pretensions of taking its place in the modern node ecosystem should have typings as a matter of course.
- unless something in the dependency tree is using it as a mainline dependency, in which case it's going along for the ride willy-nilly. But it's only a few hundred K and you won't see hide nor hair of it in the final transpiled product. It imposes absolutely no runtime penalty whatever.
4 participants
Added type definitions
Firstly - I love this little library and use all the time! 😀
But I am a big TypeScript user and find myself making a new type declarations file for each new project use it in.
It would be great to merge it into the main repo.
I would be happy to maintain this file should the api ever change.
Please consider this as I believe it adds value to a great library!
Thanks again 😄