use ts-toolbelt for 'compose' types #3563
Conversation
| @@ -69,7 +69,7 @@ export default function applyMiddleware( | |||
| dispatch: (action, ...args) => dispatch(action, ...args) | |||
| } | |||
| const chain = middlewares.map(middleware => middleware(middlewareAPI)) | |||
| dispatch = compose<typeof dispatch>(...chain)(store.dispatch) | |||
| dispatch = compose(...chain)(store.dispatch as typeof dispatch) | |||
There was a problem hiding this comment.
The next step is to get the chain properly typed so we don't need to do this casting.
| export default function compose<R>(...funcs: Function[]): (...args: any[]) => R | ||
|
|
||
| export default function compose(...funcs: Function[]) { | ||
| const compose = <Fns extends F.Function<any[], any>[]>( |
There was a problem hiding this comment.
I really wanted to use ts-toolbelt's F.compose here, but it was swallowing types. This way, I can be more explicit and get an accurate return type.
| if (funcs.length === 0) { | ||
| // infer the argument type so it is usable in inference down the line | ||
| return <T>(arg: T) => arg | ||
| // TODO: should probably throw an error here instead | ||
| return (<T>(arg: T) => arg) as any |
There was a problem hiding this comment.
TypeScript really doesn't like this line, because it goes against the return type. Rather than introducing a union type, I just cast it as any, but I think it might make more sense to just throw an error instead. Thoughts?
There was a problem hiding this comment.
I was able to make this type check in my implementation of compose. However, as I suggested in comments on that PR, I think you're right that this is strange anyhow. However, I think it should simply be statically not allowed by not providing overloads for compose that take 0 arguments rather than allowing it and throwing a runtime error.
I would go farther and say that passing compose a single function is also strange behavior and probably shouldn't be allowed, but I can see why you all might want to keep that due to the current behavior, whereas I doubt anyone has much use for or would miss it accepting 0 args.
|
brilliant! |
|
Just rebased against master. |
|
Looks like I broke master. I'll get that fixed up soon. |
|
I'd prefer to avoid adding a dependency just for one lousy function if possible. Can the compose function/type be copy-pasted or something? |
|
The compose type is way more complex than a simple copy paste. It has dependencies that have dependencies. I also foresee this toolbelt coming in handy in future PRs for improving types. I did have a copy paste branch before and it was significant enough that I welcomed the dependency approach more. What is your main concern about the dependency? If it has anything to do with footprint, it has no footprint. |
|
I generally want to minimize increasing the number of dependencies that users have to download. Sure, it's "just one more dependency", but that's how we ended up with 1500 deps being pulled down for a basic CRA+TS app. As it is, it annoys me that we have forced dependencies on |
|
The good news is it ships zero actual JS and should get fully shook out by any bundler: https://bundlephobia.com/[email protected] Unfortunately, compose is exactly the kind of function that TS wasn't built for. You basically have to build a recursive state machine inside of the types. It's really complex stuff, even to trace through with the source available. |
|
Do we have any issues with the Apache 2 license here? Concerning copy/paste. |
|
I don't think we need to copypasta into this repo. We can incur the dep because it has no impact on bundle sizes. |
|
Agreed on the bundle size thing, but this is still going to require end consumers to pull down one more extra package, right? |
|
Yes, and just one. ts-toolbelt has no deps of its own. |
|
Semi-related tangent: what is |
|
Rebased |
|
Deploy preview for redux-docs ready! Built with commit bd9b100 |
|
Just filed #3567 to discuss trying to drop all external dependencies. |
|
I opened a new PR attempting to rewrite |
|
I can do a separate copy/pasta PR when I have some more time on Monday. Have a good weekend y'all! |
|
Closing because we can't come to a consensus about the 3rd-party dependency. |
In response to #3536 (comment)
ts-toolbelt