-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Better composeMiddleware()
#65
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM I really like this -- it makes way more sense to just say what you need for deps and what you'll add at the end than try to assemble the complete env you're outputting.
Excited to see how the freeway goes.
The generation of the script is annoying but... yea this is the way it ALWAYS goes with typescript and variadic params. BTW, you should have a look at freeway cause I think it might be more than 20!
note that you appear to have a type error in CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 very nice
be569c8
to
1b5fccc
Compare
* Generate `composeMiddleware` overloads up to 30 arguments. * Use file extensions in `import`s. * Use `Simplify` in the return type of `composeMiddleware` for better ergonomics. * Add `extends {}` to the type parameters of a `Middleware` function--I don't actually remember why, but the types didn't infer correctly otherwise. * Use `const`s instead of `function`s for middleware functions. The `@type` tag doesn't apply correctly to a `function` declaration--which makes sense, since there's no equivalent syntax that would do it in TypeScript: microsoft/TypeScript#27387
The old
composeMiddleware()
didn't track types. This one does.Changes:
composeMiddleware()
cares about the types of the middleware. The type of the resulting middleware provides all context keys provided by the composed middlewares, and expects from upstream any context keys expected by the composed middlewares which aren't provided by something else within the composition.Middleware
type parameters have changed. Previously it took the type of the incoming context and the type of the outgoing context. Now it takes the type of the portion of the context the middleware requires from upstream and the type of the portion of the context that it provides to downstream. This means that two middlewares can transparently provide and consume a context key without the middleware in the middle knowing anything about it. It also means that we don't have to repeat ourselves by including the incoming context type in the outgoing context type:Middleware
requires that you pass along everything you received, so you only need to actually mention the things you're adding to the context.composeMiddleware()
, in TypeScript. The implementation is in JS with a declaration file, but the test really needs to be TS to exercise the types meaningfully. Updated thetest
script to usetsx
(no relation to the React syntax) to execute.@ts-ignore
s that, miraculously, weren't actually doing anything anymore.import().
syntax to@import
syntax for readability.This is sort of a breaking change, in that adding actual types means your types could break with the upgrade. But I'm inclined not to bother bumping a major version, because it should only break your types (at compile-time, with immediate feedback), and even then it shouldn't if you were typing things correctly to begin with.
A matching PR for Freeway will follow.