-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added type definitions #22
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rows
is optional and defaults to 1.
data: [number, number, number, number], | ||
cols: number, | ||
rows: number, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor supports an optional largeFont
option but this option missing in args
above.
@@ -23,6 +24,7 @@ | |||
"mutexify": "^1.2.0" | |||
}, | |||
"devDependencies": { | |||
"@types/node": "^11.12.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. this looks good.
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 😄