Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

beaumontjonathan
Copy link

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 😄

Copy link
Owner

@fivdi fivdi left a 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: {
Copy link
Owner

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,
Copy link
Owner

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,
Copy link
Owner

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,
});
Copy link
Owner

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",
Copy link
Owner

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?

Copy link

@daggilli daggilli Nov 16, 2020

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. this looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants