Open
Conversation
hannahhoward
approved these changes
Jan 10, 2025
Member
There was a problem hiding this comment.
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!
Member
|
note that you appear to have a type error in CI |
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.Middlewaretype 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:Middlewarerequires 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 thetestscript to usetsx(no relation to the React syntax) to execute.@ts-ignores that, miraculously, weren't actually doing anything anymore.import().syntax to@importsyntax 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.