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
Add bindMethods()
method and a mixin()
decorator (TS)
#37
Conversation
Solves #30 |
bindMethods()
method and a mixin()
decorator
Emittery.d.ts
Outdated
@@ -76,6 +76,16 @@ declare class Emittery { | |||
* specified. | |||
*/ | |||
listenerCount(eventName?: string): number; | |||
|
|||
/** | |||
* Bind a `methodNames` or all `Emittery` methods if `methodNames` not defined into `target` object. |
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.
* Bind a `methodNames` or all `Emittery` methods if `methodNames` not defined into `target` object. | |
* Bind the given `methodNames`, or all `Emittery` methods if `methodNames` is not defined, into the `target` object. |
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 you include a usage example.
https://github.com/sindresorhus/typescript-definition-style-guide#documentation
Emittery.d.ts
Outdated
/** | ||
* Returns a decorator which mixins `Emittery` as property `emitteryPropertyName` and `methodNames` or all `Emittery` methods if `methodNames` not defined into target class. | ||
*/ | ||
static mixin(emitteryPropertyName: string, methodNames?: string[]): Function; |
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 you move this to the top? I prefer having static properties first.
static mixin(emitteryPropertyName: string, methodNames?: string[]): Function; | |
static mixin(emitteryPropertyName: string, methodNames?: readonly string[]): Function; |
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.
Is there any way to make the type more specific than Function
?
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.
It is not supposed to be more specific than function, I guess. There shouldn't be any type checks on decorators value. It is pretty hard to misuse and it's not supposed to be used directly in TS anyway.
Also, as far as I understand it will require to type function as all possible numbers of arguments of constructor. And do we really need this complication for absurd level check?
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.
You're right. Probably not worth it.
Emittery.d.ts
Outdated
bindMethods(target: Object, methodNames?: string[]): void; | ||
|
||
/** | ||
* Returns a decorator which mixins `Emittery` as property `emitteryPropertyName` and `methodNames` or all `Emittery` methods if `methodNames` not defined into target class. |
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.
* Returns a decorator which mixins `Emittery` as property `emitteryPropertyName` and `methodNames` or all `Emittery` methods if `methodNames` not defined into target class. | |
* Returns a [decorator](https://github.com/tc39/proposal-decorators) which mixes in `Emittery` as a property named by `emitteryPropertyName` and `methodNames`, or all `Emittery` methods if `methodNames` is not defined, into target class. |
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.
It does not conform to this proposal. (Hint: it is a proposal, it is not implemented anywhere yet.)
index.js
Outdated
} | ||
} | ||
|
||
static mixin(emitteryPropertyName, methodNames) { |
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.
Is this following the latest decorator spec? https://github.com/tc39/proposal-decorators
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.
It is not a spec, it is a proposal. There is no implementation which can handle decorators implemented in such a way yet. I followed the typescript implementation of decorators.
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.
Alright. But it needs to be clearly documented in the type and readme that it follows the TypeScript implementation.
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.
I described it only for typescript part of readme so far(as using it from JS is weird anyway atm).
You also need to document them in the readme. Should use the same text and examples as the TS definition, to make it easy to keep them in sync in the future. |
merged master in |
bindMethods()
method and a mixin()
decoratorbindMethods()
method and a mixin()
decorator (TS)
@sindresorhus can you cut a new release and push it to npm? github master source and docs include this feature but they haven't been pushed to NPM. |
It seems to me that this feature is not really TypeScript-specific, right? Should its docs in the readme be in its own |
Fixes #30