Refactor module in TypeScript #60
Conversation
|
Yeah, makes sense for such a complex module. Thanks for doing it :) I don't use TSLint anymore, since it's being deprecated. Can you replace it with XO? See: sindresorhus/is@ea4204f |
source/index.ts
Outdated
| interval: 0, | ||
| concurrency: Infinity, | ||
| autoStart: true, | ||
| // @ts-ignore This default is a little nasty! |
There was a problem hiding this comment.
the classic old typescript problem of passing in class constructors from generics without being able to pass in types because they don't really exist 🤷♂️
There was a problem hiding this comment.
fix for now: #60 (comment)
|
Thanks for the review, I think edd0356 is my favorite commit ever 😄 Should be all good now, I had to comment out two linter rules that happened because of bugs in the linter or plugins involved such as import-js/eslint-plugin-import#1304 (just requires a version bump somewhere since it has been merged already) and someone to tell |
package.json
Outdated
| "call-signature", | ||
| "property-declaration", | ||
| "member-variable-declaration" | ||
| ] |
There was a problem hiding this comment.
Can you explain these rule changes?
There was a problem hiding this comment.
Also due to a rule in the old tslint config, it required
() => {}to have a defined return type but had the conflicting rule of no redundant typedefs since it could be inferred. Good we dropped that anyways 🔧
source/index.ts
Outdated
|
|
||
| private _resolveIdle: ResolveFunction = empty; | ||
|
|
||
| constructor(opt?: Options<QueueType, EnqueueOptionsType>) { |
There was a problem hiding this comment.
that requires a typecast (or custom typeguard when using a method) to tell ts it can't be undefined anymore thus helping out since it allows us to drop the @ts-ignore 🎉
|
Looks good. Thanks for doing this, @ltetzlaff :) |
Hi there, I figured I use this module quite a bit and at some point wasn't sure whether its internals were working correctly (spoiler: they did) so I added some types to make debugging easier and thought it might be worth giving a little more attention.
I used the nice and verbose jsdoc summaries by @BendingBender and realised on the way I can also resolve this discussion #57 (review) by using basically just one line here
extends EventEmitter<'active'>👋