Update generic signature of `addReaction`

[amcclain]

• Supports resolving the generic T for multiple ReactionSpec<T> args passed to this function, where T is inferred from the return type of the track function within each spec, individually.

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be checked off to indicate they have been considered. If unclear if a step is relevant, please leave unchecked and note in comments.

• [x] Caught up with develop branch as of last change.
• [ ] Added CHANGELOG entry, or determined not required.
• [ ] Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
• [x] Updated doc comments / prop-types, or determined not required.
• [x] Reviewed and tested on Mobile, or determined not required.
• [x] Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to collapse multiple intermediate commits into a single commit representing the overall feature change. This helps keep the commit log clean and easy to scan across releases. PRs containing a single commit should be rebased when possible.

[amcclain]

I think this is worth doing, although I would want to ensure it doesn't create any issues.

I noticed that despite the generics in the signature for addReaction and ReactionSpec, the return type of my track function was not being set into the generic and the run function arg type was always any. I believe we lost the link when we moved to accepting multiple specs in a single call to addReaction (which for the record was a change I loved).

Example from FilterChooserModel, with the IJ inlay types showing the difference:

before

after

Note that the common pattern of returning an array of mixed objects from track works, but arg is an array of the union type of all members, which is minimally useful.

I am not super-attached to this change, but it does feel like it closes a gap w.r.t. all the typing we already have pulled through this complicated little function.

[amcclain]

Oh meant to post this more interesting example - with multiple specs (kinda the point) - from BaseMembersModel within role admin code:

before:

after:

[amcclain]

Last thought - we would want a changelog entry if we merge, as I can imagine at least some type issues being caught here. We should check to see if the array pattern ends up being actively harmful if an element from that array is passed into a typed function signature - it might require an assertion where before it did not (due to implicit any defeating all type checking)

[lbwexler]

Thanks for the excellent examples -- would not have understood the value of this so quickly otherwise

[lbwexler]

This is so clever -- never would have figured it out. But two quick thoughts --

1) If we do go with this, would we consider delivering the typing as one or more function overloads as follows? Might make it easier for Javascript readers to grok the basic signature?

addReaction<T>(t: ReactionSpec<T>): IReactionDisposer; addReaction<T extends any[]>(...specs: {[K in keyof T]: ReactionSpec<T[K]>}): IReactionDisposer | IReactionDisposer[]

addReaction(...specs: ReactionSpec[]): IReactionDisposer | IReactionDisposer[] { // Implementation }

2) There is also the following pattern to consider. Don't think I fully understood the concerns in your final comment, but perhaps the following would work better for most cases?

addReaction(t1: ReactionSpec): IReactionDisposer; addReaction<T1, T2>(t1: ReactionSpec, t2: ReactionSpec): IReactionDisposer[]; addReaction<T1, T2, T3>(t1: ReactionSpec, t2: ReactionSpec, t3: ReactionSpec): IReactionDisposer[]; addReaction<T1, T2, T3, T4>(t1: ReactionSpec, t2: ReactionSpec, t3: ReactionSpec, t4: ReactionSpec): IReactionDisposer[]; addReaction<T1, T2, T3, T4>(t1: ReactionSpec, t2: ReactionSpec, t3: ReactionSpec, t4: ReactionSpec, ...specs: ReactionSpec[]): IReactionDisposer[];

`

`

[ghsolomon]

Lee I had the same thought! I think this all looks great and is a big improvement.

[amcclain]

For the record - further commit https://github.com/xh/hoist-react/commit/0df233bac4cd483d02012dd2547f6efd25b4aaf4 tweaked type signature and added CL entry w/warning re. possible new tsc errors