Rewrite compose utility function

[joshburgess]

I saw you discussing pulling in ts-toolbelt to rewrite the compose utility in #3563 and asking for alternative solutions on Twitter. I don't think you need to pull in a utility library to do so this, and ts-toolbelt's solution seemed more complex than necessary, utilizing a bunch of custom type-level helper utilities. So, I decided to take a shot at rewriting compose from scratch, using no external dependencies & avoiding any entirely. This is what I came up with.

Important notes:

• There are no behavior changes other than a change to the generic type params used in various overloads. When explicitly providing generic params for overloads accepting multi-arg functions, you must now pass in a tuple of types for the generic instead of only the type. This is accomplished via the combination of A extends unknown[] & (...args: A). This actually fixes a bug that was present before which (accidentally, I believe) restricted these multi-arg functions to take in arguments all of the same type, which isn't necessary. You can see the changes I'm talking about reflected in the updated test.

• I did not change the actual logic in the implementation body of the function. We could rewrite it without reduce in a potentially more performant way, but I wanted to keep the changes in this PR restricted to how the types work so that it would be easier to review. Note: There are some changes in the implementation, but they are just related to type signatures. Also, we could use Function where I'm using Fn and unknown[] where I'm using Parameters<typeof b> if you don't like what I did with these. It would still work the same way.

• I think what's in the PR now is an improvement and could be merged as is (after fixing the line passing ...chain to compose that's currently causing the build to fail), but, while I kept the actual functionality/behavior the same in this PR, I think it's worth considering making some further changes beyond what's been done so far in order to simplify what's required to properly type this function. I'll add more info on this in a followup comment.

[netlify]

Deploy preview for redux-docs ready!

Built with commit 89c11f3

https://deploy-preview-3568--redux-docs.netlify.com

[joshburgess]

Things worth thinking about possibly changing:

• If you look at the overloads, you will see a set of them under this comment:

// extra overloads allowing functions other than the right-most function
// to take in more than one argument, though the extra arguments go unused

...overloads here...

I had to add these overloads because various tests were passing in functions, other than the right-most function, which took in multiple params. This behavior was allowed previously due to the types (and I've maintained that behavior in this refactor), but it actually goes against what the JSDoc comment description said: Composes single-argument functions from right to left. The rightmost function can take multiple arguments .... Personally, I would use the type system to not allow the user to give compose functions that take multiple arguments, since the implementation completely ignores the extra args anyway. I think the way it works right now is a bit misleading. If we removed this odd behavior & changed those tests, we could get rid of those extra overloads and have simpler typings for compose.

• If we consider further breaking changes, does the right-most compose really need to take in multiple arguments? This is abnormal versus how compose functions work in most other utility libraries that have it. I understand you might want to support it to make things easier to use, but the typings would be much simpler & more straight forward if all functions passed to compose just took a single argument, and the user could still accomplish the same things by just partially applying some arguments to the right-most function using wrapper functions & currying instead. They have to do this already with the other (non-right-most) functions anyway since only the first arg passed to them gets used. It's worth thinking about, IMO.

• Why allow compose to take in no arguments and 1 argument? Again, this behavior is a little strange compared to what you typically expect from a compose utility. It seems to be written in a way that is too dynamic and overly complicated, IMO. While it was possible to provide valid typings for the current behavior, these corner cases made things more complicated & required more overloads. Conceptually, it doesn't really make much sense to allow this function to take 0 or 1 args. Not restricting this in the type system could actually be worse for the user by allowing them to accidentally do things they may have not intended to do, IMO. Passing one function just gives you that same function right back, which isn't very useful. Worse, passing no functions to compose gives you back a function taking any number of arguments and returning the first argument it's passed. If memory serves me correctly, this is different than even what ramda's compose function does. I believe it just returns itself when given no args. Either way, both of these things are a little strange and not very useful.

• Regardless of whether any of the above things change or not, why does the library offer only a right-to-left composition utility and not left-to-right? It would be pretty straight forward to add a complementary pipe/flow/composeFlipped/whatever-you-want-to-call-it function that worked the same way as compose but in the opposite direction. I prefer right-to-left composition in languages where it's normal & there are no problems with type inference, but TypeScript (and Flow) generally has a much easier time with left-to-right inference. Even pretty serious functional programming libraries, like fp-ts, have decided to deprecate right-to-left versions of these types of utility functions & promote the use of left-to-right versions instead due to stronger inference capabilities. I'm not necessarily suggesting to remove compose altogether, but if you keep it, why not add a complementary left-to-right composition function to go along with it for people who would rather use that?

Anyway, these are just some thoughts I had while/after implementing the changes for this PR. I know most of the things mentioned above are a result of code that was written before any of the current maintainers were maintaining the project, but, considering you're already rewriting in TypeScript and will have some breaking changes anyway, I think it's worth thinking about some of these things and possibly making some changes in an effort to simplify behavior & make things easier to statically type.

[markerikson]

Thanks for filing this!

To answer your last question briefly, Redux's compose is really only used for two cases: store enhancers and HOCs. In both cases, the reading order matches the nesting:

const composedEnhancer = compose(
    applyMiddleware(thunk, logger),
    batch(),
    devTools()
);

const composedComponent = compose(
    connect(mapState, mapDispatch),
    intl(),
)(MyComponent);

There's no need for varying argument orders here.

Specifically to the "only one argument" question: there's a lot of Redux-using code out there that is calling compose(applyMiddleware(...middleware)). A common example would be code like this, which is shown in the Redux DevTools Extension docs:

const composeEnhancers =
    window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__ ?
        window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__({}) : compose;

const enhancer = composeEnhancers(
    applyMiddleware(thunk),
);

Note that the DevTools Extension compose() always adds the DevTools enhancer itself as the last enhancer in the chain.

So yes, we do indeed want to keep the 1-arg case. Agreed that the 0-arg case is kinda silly, though.

Not sure on your other questions. I'm sure there's prior discussion wayyyyyy back in the issues/PRs history that might be relevant.

[joshburgess]

Note: The build is currently broken due to this line:

dispatch = compose<typeof dispatch>(...chain)(store.dispatch)

This will need to be fixed. I think part of the problem is related to other types in this file, but there's also the fact that chain is an Array of any length here, whereas this PR's implementation of compose only allows overloads for up to 5 functions.

[markerikson]

Hmm. Can it be made generic for an arbitrary number of functions? Realistically I don't expect too many, but that was part of the genesis for pulling in ts-toolbelt in the first place (see discussion at #3536 (comment) and following ).

[joshburgess]

@markerikson I'm not sure I follow about the reading order matching the nesting, but I can see why a left-to-right version or eliminating the 1-arg case would complicate things since the dev tools' compose is also right-to-left & has the same 1-arg handling as the current behavior. I think, ideally, neither the devtools' compose nor this one would allow 0 or 1 args and people would just not wrap their code with compose in those cases, but I can see how coordinating this with the dev tools maintainer would be difficult. So, it's probably not worth trying to change.

About my other suggestions: I think the most important one is the first about not allowing the non-right-most functions to take more than one argument. I can't think of a good reason for allowing this, since the implementation ignores them anyway. I think it would be best to just not allow people to pass these kinds of functions to compose at all.

[joshburgess]

Hmm. Can it be made generic for an arbitrary number of functions?

I'll look into it. I also haven't played with ts-toolbelt at all. I hadn't heard of it until seeing you all talk about it. I'll try it out.

[markerikson]

Yeah, lemme phrase it this way: there's a ton of existing code that we don't want to break, compose has a limited number of use cases with Redux, and we're looking for practicality over idealism or purity.

Agreed that multi-args probably aren't relevant, but I don't know how we'd prevent that at the JS level (and not entirely sure how you'd do it at the TS level). Not terribly critical.

[joshburgess]

Okay, @markerikson I updated the PR with a new overload based on what @jedmao was doing in the other PR, but without using ts-toolbelt. I replaced my Params util with the native Parameters, which I didn't know existed before (I usually defined something like this myself in my own TS projects), and then used that along with the native ReturnType and some new type-level utils to add a new generic overload for any number of functions.

However, I didn't remove any of the other overloads, as I feel like this should be a last resort sort of solution. Typically, you'd still want the help of the inference of the other overloads. This is really only needed for that one spot where compose is used internally in applyMiddleware. Maybe, that bit of code should be rewritten to not use compose instead?

Also, we could raise the number of functions from up to 5 to up to 10 to cover more ground, if you think it's necessary? It obviously adds a good bit of length to the definition, but it's fairly mechanical.

[cellog]

I am unclear how this is better than what we have now - the point of the rewrite was to remove the overloads, this just replaces them? Can you explain what makes this better than the existing implementation? I am sure you already have, but I was not able to parse it out from the thread above, and perhaps a simple summary would help me

[nickserv]

If we're going to have this generic type signature anyway, why not delete all the others?

export default function compose<Fns extends Fn[]>(...funcs: Fns): Composed<Fns>

[markerikson]

I'm inclined to agree with Greg. The original genesis of the question was if there was a way to handle an arbitrary number of arguments, generically, without hand-specifying specific numbers of parameters in a copy-paste manner. At the moment, this just looks like it rewrites the manual overloads, and doesn't simplify / replace them.

[markerikson]

@OliverJAsh pointed to his pipe-ts lib as an alternative implementation:

https://github.com/unsplash/pipe-ts

However, that one also has hardcoded overloads, although it goes up to 9. (but why not 11? :)) He thinks that an arbitrary implementation isn't possible atm:

https://twitter.com/OliverJAsh/status/1170753063709224961

also, apparently it's a left-to-right impl, so nm anyway :)

[joshburgess]

If we're going to have this generic type signature anyway, why not delete all the others?

export default function compose<Fns extends Fn[]>(...funcs: Fns): Composed

Like I said, this should be a last resort. It doesn't provide the same inference as the overloads because of the nature of how it works. All it does is create the return type of the composed function by getting the params of the right-most function and the return type the left-most function:

type Composed<Fns extends Fn[]> = (
  ...args: Parameters<Last<Fns>> // params of right-most function
) => ReturnType<Fns[0]> // return type of left-most function

This says nothing about the types of the functions you're passing in though. You basically lose all inference help for functions in between the first and the last and now must manually annotate everything. In this PR, try commenting out all of the overloads except for the generic one you suggested & the base case with the implementation body. Then, in the tests, find a compose call taking in at least 3 functions, choose a function in the middle and replace it with an unannotated version (defined above or defined inline). You'll find that you have no type inference at all and the variables in your unannotated function get inferred as any.

This makes sense, because the generic implementation function compose<Fns extends Fn[]>(...funcs: Fns): Composed<Fns> knows nothing at all about this function or how types are propagated through each function. It only knows what the result of composing all of them together should be.

Also, there are problems with the old overloads. I called out an example in the original post: This actually fixes a bug that was present before which (accidentally, I believe) restricted these multi-arg functions to take in arguments all of the same type, which isn't necessary. Because of the way generics were being used, restrictions were being applied in ways that didn't need to be there.

For example, in this test: expect(compose<number>()(1, 2)).toBe(1), the overload for this case forced all params to be of type number, but neither this case or any of the other cases that take multiple params, actually need to do this. They should support params of different types. That's why using the A extends unknown[] + (...args: A) style of overloads is better. It provides better inference & more flexibility with overload signatures.

Another thing to try... Underneath the above mentioned test, try adding this line:

expect(compose()('zero', 1, 2)).toBe('zero')

You will get a type error because there is no overload that properly infers arguments without manually/explicitly providing generics for this case. This isn't true with the overloads in my PR. They are able to provide correct inference without explicitly annotating for many cases.

I think people are looking for an easy, simple solution where one doesn't exist. Thoroughly typing a generic, variadic version of compose with good inference has been a problem in TS for a very long time, and it's still not really solved in a small, elegant, generic way by any library right now. As @jedmao noticed, you lose type information with ts-toolbelt's Compose utility (you'll even see in comments and commit messages that they already know they have trouble with propagating generics in various situations), and you lose inference in various ways here with the params of last function + return type of first function approach.

This is why libraries like fp-ts (which has some of the best inference capabilities of any library with utilities like these) take the pragmatic/practical approach of just specifying the exact overload signatures for a reasonable amount of arguments (usually 5-10). A function with a ton of boilerplate of carefully written overloads that guides you along & helps you to prevent errors through proper inference & type propagation ends up being a lot more helpful/useful than a small, elegant solution that loses type information & is unable to provide you proper inference.

Yes, requiring a ton of manually written overloads sucks, I know, but, unfortunately, as far as I know, it's the only way to really make things work the way they should in TS.

[nickserv]

If it's possible, it would be better to use mapped tuple spread types so we can make this type safe and homomorphic.

[markerikson]

That's what I thought I was suggesting in the other thread when I first brought this up :)

[joshburgess]

If it's possible, it would be better to use mapped tuple spread types so we can make this type safe and homomorphic.

Hmm... I think I would need to see an example. I'm not tracking what you mean.

Don't get me wrong, I would love if someone were able to solve this problem without inference problems & tons of overloads, but I haven't seen it yet.

[jednano]

To be clear, the ts-toolbelt actually uses like 40 overloads under the hood, so hiding that in a separate library does make the code more readable, FWIW.

[nickserv]

I checked the types from Underscore and Ramda, and they don't do this. As far as I can tell, this is not possible with TypeScript yet. I can do the following the a fixed number of arguments to homomorphically detect the return type in TypeScript 3.4, but since spread types aren't Turing complete I can't generalize this to variable arguments without breaking the homomorpism:

function compose<A, B, C>(f: (arg: A) => B, g: (arg: B) => C): (arg: A) => C {
    return x => g(f(x));
}

Source: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html

[nickserv]

Compose.sync from ts-toolbelt seems like an improvement as it can infer the arg types as the last function's args and the return type as the first function's return, but it doesn't seem to match the types in between so I don't know if you can call this a homomorphism: https://github.com/pirix-gh/ts-toolbelt/blob/3b072630/src/Function/Compose.ts#L75

export type Compose<Fns extends Function[], mode extends Mode = 'sync'> = {
    'sync' : (...args: Parameters<Last<Fns>>) => Return<Head<Fns>>
}[mode]

[joshburgess]

@nickmccurdy Yes, that's what I was saying. It loses type information/there is no inference for the function args. It only knows how to create the return type, but this requires you to annotate all functions, and leaves open the possibility for masked mistakes.

To be clear, the ts-toolbelt actually uses like 40 overloads under the hood, so hiding that in a separate library does make the code more readable, FWIW.

@jedmao What do you mean? ts-toolbelt's F.Compose does not provide explicit overloads. It works essentially in the same way as the Composed helper I defined here and has exactly the same problems.

I'm just reiterating the things I've already said in previous comments, but just to be extra sure, I just created a new branch, installed ts-toolbelt and used its F.Function and F.Compose utils to define compose and looked over the tests, and all the same issues are still present.

I went ahead and modified expect assertions + left a bunch of comments to try to show the problems with using this approach as the sole type definition without overloads.

You can find the branch here:
https://github.com/joshburgess/redux/tree/ts-toolbelt-compose

compose function defined in terms of ts-toolbelt's F.Function & F.Compose:
https://github.com/joshburgess/redux/blob/ts-toolbelt-compose/src/compose.ts

Test file with modifications & comments showing the problems with this approach:
https://github.com/joshburgess/redux/blob/ts-toolbelt-compose/test/compose.spec.ts

This is why using explicit overloads is better, IMO. It gives you much stronger inference, and, thus, is much safer. This generic definition is not safe on its own. It only really makes sense as a last resort overload to fall back on when no other overloads can be used to try to give the user a small amount of type information, because that's better than nothing (I suppose).

[cellog]

To be clear, the ts-toolbelt actually uses like 40 overloads under the hood, so hiding that in a separate library does make the code more readable, FWIW.

@jedmao What do you mean? ts-toolbelt's Compose does not provide explicit overloads. It works essentially in the same way as the Composed helper I defined here and has exactly the same problems.

I'm just reiterating the things I've already said in previous comments, but just to be extra sure, I just created a new branch, installed ts-toolbelt and used its F.Function and F.Compose utils to define compose and looked over the tests, and all the same issues are still present.

I went ahead and modified expect assertions + left a bunch of comments to try to show the problems with using this approach as the sole type definition without overloads.

You can find the branch here:
https://github.com/joshburgess/redux/tree/ts-toolbelt-compose

compose function defined in terms of ts-toolbelt's F.Function & F.Compose:
https://github.com/joshburgess/redux/blob/ts-toolbelt-compose/src/compose.ts

Test file with modifications & comments showing the problems with this approach:
https://github.com/joshburgess/redux/blob/ts-toolbelt-compose/test/compose.spec.ts

This is why using explicit overloads is better, IMO. It gives you much stronger inference, and, thus, is much safer. A type like F.Compose is not safe on its own. It only really makes sense as a last resort overload to fall back on when no other overloads can be used to try to give the user a small amount of type information, because that's better than nothing (I suppose).

Perhaps we aren't using ts-toolbelt properly?

https://github.com/pirix-gh/ts-toolbelt/blob/master/src/Function/Compose.ts#L45

This code clearly infers the returns and parameter types of every intermediate function. I have to run, but I will write more later

[joshburgess]

Perhaps we aren't using ts-toolbelt properly?

I think you're right. Sorry, I didn't read those comments before. I see they are using mapped types to try to infer more information via the tuple's indexes. That looks interesting.

This code clearly infers the returns and parameter types of every intermediate function. I have to run, but I will write more later

I don't think that's true. You can see in the example that they annotated all of those functions manually.

I updated with a new branch using the F.Composer + F.Compose approach like the example in the comment there showed. This broke the 3 cases in the implementation (0 args, 1 arg, > 1 args), and I don't have time to fix them right now, so I just asserted them as any for now. (These types do not ever leak outside of this function anyway. So, it doesn't influence anything outside of here.)

New branch here:
https://github.com/joshburgess/redux/tree/ts-toolbelt-composer-compose

compose:
https://github.com/joshburgess/redux/blob/ts-toolbelt-composer-compose/src/compose.ts

Test file:
https://github.com/joshburgess/redux/blob/ts-toolbelt-composer-compose/test/compose.spec.ts

I had to update the test file to change some functions, as this way of defining compose does not allow any of the functions to take in multiple arguments. So, even if this worked 100% without issues, you wouldn't be able to use it directly.

However, it looks like it's still unable to properly propagate types through the function args, as you can see the things that were any and never before are still the same.

[nickserv]

Explicit overloads are not homomorphic. I think we’re better off with TS-toolbelt’s compose function.

[cellog]

OK, I did some experimenting, you can see the results here.

Note that the copy-pasta to make this work is pretty insane (first of all).

Also, note the pure simplicity of the compose function, no types need be defined at all.

Second, it demonstrates that in fact the types are correctly inferred for intermediate functions that explicitly define their types.

However, I think I see the disconnect now. This definition of compose expects the intermediate functions to explicitly declare their parameter types. The redux implementation has been more strict, requiring all of the intermediate functions to have the same signature (all middlewares, for instance, or all store enhancers).

This then raises a simple question: is this bad? What is the issue with requiring middlewares and enhancers to be explicitly typed?

[timdorr]

I assume when people need to compose a set of general functions, their first thought isn't to reach for Redux.

But perhaps we can be explicit about this by renaming the function to composeEnhancers (and re-exporting under the compose name for BC).

[joshburgess]

Second, it demonstrates that in fact the types are correctly inferred for intermediate functions that explicitly define their types.

I think people were (are?) misunderstanding the things I wrote for some reason. We are talking about two different things here. You're saying that it's able to infer the correct return type for compose, but I've already said & shown that in the previous posts. I'm talking about the functions you actually give to compose as arguments.

The reason why I'm advocating for overloads is specifically because the types of the functions being passed to compose can not be properly inferred based on the functions to the right of them, which is unsafe and allows anys to sneak in. You can see that it doesn't carry over any information at all between functions. Even when the right-most function is explicitly annotated, it doesn't know what the input param types of the function to the left should be. The tests file in my last post shows this. In my view, that means that this solution is inherently worse than explicitly providing overloads which provide much better inference and safety.

This then raises a simple question: is this bad? What is the issue with requiring middlewares and enhancers to be explicitly typed?

I think so, yes, because it could provide much better inference. The issue isn't whether or not explicitly typing things is bad. It's whether or not the function can allow you to write the intermediate function arguments inline without explicitly annotating every one of them, which is very much possible with a different implementation, like what's in this PR. In my opinion, disregarding this just to try to achieve an elegant one type signature solution that doesn't cover all cases and has worse type inference is a bad tradeoff.

If you all disagree with that, then there's nothing more I can say, but I think that is contrary to what most people using TypeScript want & expect.

[jednano]

I agree. If we can provide better inference and not swallow types along the way, even if that means an insane number of overloads, the user experience should be the most important goal here.

My ts-toolbelt PR was meant to do both, but if this PR identified and fixed some holes, I'm all for it!

[cellog]

This makes sense to me Josh, thanks for the extra clarity. I think the key difference is in assumptions.

I am assuming that our compose function has only 2 limited uses: composing middlewares and composing store enhancers, as this is the two ways it should be used. In both cases, functions will never be defined inline to the actual compose definition.

If we assume it should be general purpose, then my assumptions break down.

A larger problem is the definition of applyMiddleware, which also currently is a forest of overloads. To properly redefine that, we will need to write our own definition of the compose types anyways.

In any case, I think we are getting closer to a solution, even if we ultimately decide not to touch the existing definitions. Understanding the beast we are up against is equally important.

The key takeaway I have from all of this is that typescript really needs to be improved drastically before it can truly handle the expressiveness JavaScript supports in a clear way, and that is a rather large asterisk on the statement "Typescript provides type safety for JavaScript" that we should all be cognizant of.

Now, before I get too philosophical, I wonder if we can take a step back and answer a couple of basic questions:

1. What are the limits we expect to place upon the inputs to composeEnhancers (h/t to @timdorr this is a suggestion I like)?
2. How about to applyMiddleware?

I have answers but rather than bias the results I would like to hear from others first.

If, for example, we want a maximum number of middlewares or of store enhancers to support, that would create a limit we can work with. Or, if we expect all params to be the same signature, that also gives us a useful limit. Some of this is implicit (all middlewares should have same sig) but let's make it explicit.

Then, we should design types that support these limits.

[jednano]

We could also introduce two separate compose functions that are meant for either scenario, which might allow more freedom in how each is typed.

[markerikson]

The other common use case I've seen is composing HOCs.

[timdorr]

I think React Redux should provide its own compose for that, then. That would let whomever is writing the typings for React Redux also avoid this mess, because an HoC has a specific and consistent type ((args: T) => (comp: React.Component): React.Component => comp)

[FireyFly]

I'm guilty of using compose from Redux for non-Redux things (most commonly composing HOCs, be it including React Redux connect or not). My reasoning for that has been that it's provided by Redux anyway, and the documentation I think quite clearly states/suggests that it's just ye olde functional-programming compose anyway.

I don't really have a strong opinion on the issue topic as a whole, but at least to me it's made sense to just reach for it as a general-purpose compose in a project that already depends on Redux anyhow. One potential concern is that if the function remains exported as compose but the TS typesig restricts it to Redux uses (of composing store enhancers etc), then that might add friction to migrating a project to TypeScript from vanilla JS.

[jednano]

Honestly, I think compose should be an external dependency from Redux, but all the compose functions on npm seem to be not what we want, let alone typed properly.

Everyone seems to have a different idea about the implementation of compose. Looks like they use promises pretty commonly too.

[markerikson]

It's part of our current API, everyone depends on it, and we're not going to remove it for v5.

[timdorr]

Why not? It's the perfect time to make a breaking change.

[cellog]

I'd recommend deprecating it for a while before removing it

[cellog]

or (sorry to not have thought of this before I hit send), re-export it from somewhere else and deprecate it. Make the external dep a peer dep, so that when it is finally removed, the user still has the peer dep

[markerikson]

I strongly oppose removing it.

The compose function we have works. It's just a question of the best way to type it.

There's no reason to remove it, and we would break tens of thousands of applications by removing this function.

I'm fine with fiddling with type declarations for v5, but the point of the conversion was that we would not change any actual JS functionality.

[nickserv]

To be honest, I don't think we have enough TypeScript contributors to keep the compose types up to date at the level that it should be, especially considering that new TypeScript will add advanced new type features that we should adapt to. Compose should be removed so we can get the best types from another library. Simply adding types to our existing JS code isn't enough, TypeScript won't be able to infer or verify the types.

[timdorr]

I'm not saying remove the function itself, just rename it to its specific use (composeEnhancers) and type it as such.

[cellog]

OK. Here's what I propose: we do nothing. The types, as they are, work fine. The only goal here was to improve them. AFAIK, no one has had trouble with the existing types.

Perhaps later Typescript will make this easy. Until then, I don't see any obvious benefits to changing things, and many disadvantages.

[jednano]

I know for certain there are people importing redux just for its compose function. They aren't even using Redux! I think that's a pretty good indicator that it doesn't solely belong in the redux package. If redux re-exports it, that makes more sense, but then it turns into "another dependency," which is a different conversation entirely.

This conversation has surely turned into a 🐇 🕳 , so I'm with @cellog. I'll be closing my PRs.

[nickserv]

We should delete compose to let it be maintained by other packages, then Redux users can install the other packages and non-Redux users won't need to install Redux just for compose.

[markerikson]

NO.

Again, I am not going to break the Redux public API, even in a major, just for the sake of some notion of "letting people bring their own compose function".

If folks are installing Redux just to get compose, that's their problem, not ours.

[ematipico]

Why removing an API just because a tool (TypeScript) is not able to do what's needed?
There should be a good reason to deprecate => remove an API. Reasons around the library itself, not a tool. Right? Or did I miss something?

[jednano]

The argument to extract the compose function outside of the library has nothing to do with TypeScript, which means it's out of scope from this thread.

[markerikson]

Here's my stance atm:

• We are not removing compose
• We are not renaming it
• We are not making it an error to pass in 1 argument
• If there are any other legitimate improvements we can make to the types without pulling in other libraries, I'm interested, whether it be based on multiple overloads or some kind of mapped tuple whatevers

Otherwise, we'll just stick with what we've got and I'll close this.

[timdorr]

I'm just going to close this regardless. There are some potential avenues to explore, but it doesn't sound like this PR is one of them.

[arecvlohe]

It would have been an improved developer experience to have more correct type inference.

[joshburgess]

Why was this closed?

@markerikson just said this:

If there are any other legitimate improvements we can make to the types without pulling in other libraries, I'm interested, whether it be based on multiple overloads or some kind of mapped tuple whatevers

yet this PR fixes bugs in the existing overloads & doesn't pull in any external dependencies.

From a previous comment in this thread:

Also, there are problems with the old overloads. I called out an example in the original post: This actually fixes a bug that was present before which (accidentally, I believe) restricted these multi-arg functions to take in arguments all of the same type, which isn't necessary. Because of the way generics were being used, restrictions were being applied in ways that didn't need to be there.

For example, in this test: expect(compose<number>()(1, 2)).toBe(1), the overload for this case forced all params to be of type number, but neither this case or any of the other cases that take multiple params, actually need to do this. They should support params of different types. That's why using the A extends unknown[] + (...args: A) style of overloads is better. It provides better inference & more flexibility with overload signatures.

Another thing to try... Underneath the above mentioned test, try adding this line:

expect(compose()('zero', 1, 2)).toBe('zero')

You will get a type error because there is no overload that properly infers arguments without manually/explicitly providing generics for this case. This isn't true with the overloads in my PR. They are able to provide correct inference without explicitly annotating for many cases.

FWIW, regardless of whether or not it happens (sounds like it won't), I agree with the point that a few people made: Redux shouldn't have a compose function. Even without the static typing issues, it never really made sense for the library to include this function as an export in the first place, because it's a common thing that already existed in various utility libraries.

And, while TypeScript is far from the most powerful or most well designed type system out there, I want to push back on this

The key takeaway I have from all of this is that typescript really needs to be improved drastically before it can truly handle the expressiveness JavaScript supports in a clear way, and that is a rather large asterisk on the statement "Typescript provides type safety for JavaScript" that we should all be cognizant of.

a little bit. These kinds of problems pop up often when JS code is written in a too loose, too dynamic way. The definition for compose in most languages is a simple function that takes two arguments, a function b -> c and a function a -> b, and returns a new function a -> c, and that's it. No variadic input args, no producing a function that accepts multiple args or taking in intermediate functions requiring multiple args (though this would be fine if they were curried), no calling it with 0 args, no calling it with 1 args, etc... just simple binary function composition. Anything with such a dynamic, loose spec is always going to be difficult to statically type. Many people would say that it's, arguably, not well designed.

Rewriting the library in TypeScript and preparing a major version update is the perfect time to fix the problems, either by removing the function from the library entirely or drastically simplifying it to make it more sound and easy to statically type. There is really no reason not to do one of these things other than just doubling down on historical mistakes.

By the way, without rewriting the code in applyMiddleware that uses compose, I think you are going to have a hard time statically typing it well. Right now, in the master branch, the type of chain is:

((next: Dispatch<AnyAction>) => (action: any) => any)[]

and the type of the function returned by compose<typeof dispatch>(...chain) is:

(...args: any[]) => Dispatch<AnyAction>

These are very loose types.

[markerikson]

sigh

WHY IS EVERYONE SO INTENT ON REMOVING compose FROM OUR PUBLIC API?!?!?!?

I honestly don't get the reasons why folks are saying that, and I don't know how much clearer I can be that WE ARE NOT GOING TO REMOVE compose!.

This is why I get so frustrated with FP purists (as a general observation). Pragmatism, practicality, and working code matter, people.

This isn't some theoretical issue we're talking about. If we remove compose, people's totally valid working code is going to break, for no good reason whatsoever. I refuse to do that.

Specifically to the "why is compose even in the lib in the first place?" question: Technically speaking, Redux doesn't even "need" to include combineReducers, and even applyMiddleware could have been a separate lib. They were included because they both are expected to be commonly used, and we wanted to encourage people to make use of these as the default approach.

The biggest weakness with store enhancers is that createStore accepts a single enhancer as an argument. That's okay if you're just calling applyMiddleware(), but the instant you want to do something more (like set up the Redux DevTools, a core selling point of using Redux in the first place), you have to compose(applyMiddleware(), devTools()). So, again, it's included because we expect people to need and use it frequently.

Tim closed the PR because it sounded as if the approach here wasn't really adding anything useful. It's possible that's not the case, but the discussion in this thread has gotten out of hand.

Per my prior comment, I'm still open to actual valid changes that improve the types, but if people keep yelling about removing compose or "fixing" it in ways that break working code, I'm gonna pull the plug on this entire idea. Stop it.

On that note, I'm gonna lock this PR. I am legitimately requesting new PRs that actually improve the types, and let's pick up the discussion fresh on one of those, focused on just how to make the types better.