refactor: bindCallback and bindNodeCallback are based on the same…
#5781
Conversation
… code - Add `bindCallbackInternals`, a base implementation for both creation methods - Add a missing test to `bindNodeCallback` to catch an issue where the initial call to the provided function was not scheduled. - Updates comments
| @@ -255,98 +252,10 @@ export function bindNodeCallback(callbackFunc: Function, scheduler?: SchedulerLi | |||
| * deliver. | |||
| * @name bindNodeCallback | |||
| */ | |||
| export function bindNodeCallback<T>( | |||
| export function bindNodeCallback( | |||
| callbackFunc: Function, | |||
There was a problem hiding this comment.
issue: Not related to the refactor, but can we get rid of the Function usage while we're here? It's a completely useless type. IIRC, it's something like { name: string }. (I think I'll write a lint rule, now, to ban it.) (...args: any[]) => any would be better. Maybe we should have an internal type for an arbitrary function?
I can see that the bindCallback and bindNodeCallback implementation also make liberal use of any instead of an arbitrary function type. They should be fixed up, too, IMO. Maybe in another PR.
There was a problem hiding this comment.
Sure. I think it's just there from "way back when" TBH. I was pretty new to TypeScript whenever that was written.
There was a problem hiding this comment.
I'll leave it for another PR though.
… code
bindCallbackInternals, a base implementation for both creation methodsbindNodeCallbackto catch an issue where the initial call to the provided function was not scheduled.This work effectively just copies and pastes the work from #5780 into a new function called
bindCallbackInternals, and alters it a bit to account for the differences in the call patterns. Then it adds the use ofsubscribeOnto the scheduled code path, becausebindCallbackhad a test that caught something that the tests forbindNodeCallbackdid not. Also adds that test forbindNodeCallback.