Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

Peeja
Copy link
Member

@Peeja Peeja commented Jan 9, 2025

The old composeMiddleware() didn't track types. This one does.

Changes:

  • New 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.
  • The types are generated from a script; this is because, unfortunately, TS is currently unable to make the types of tuple members depend on one another, so the only way to make this work is to provide explicit overloads for each arity. (Library implementations of similar functions do the same thing.) Generating them from a script ensures we don't mess up one of them. I've set it to generate 20, which should be plenty and not be a burden on the type checker. I've also committed the generated code, because it would be a pain to generate constantly, and it should rarely change.
  • The 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.
  • Added tests for 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 the test script to use tsx (no relation to the React syntax) to execute.
  • Documented a few things that didn't have docs.
  • Removed some @ts-ignores that, miraculously, weren't actually doing anything anymore.
  • Converted a bunch of import(). syntax to @import syntax for readability.
  • Added Prettier using the config from our other repos, because the code generation uses it to create better looking output. If it's a pain, we can add some ignores.

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.

Copy link
Member

@hannahhoward hannahhoward left a 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!

@hannahhoward
Copy link
Member

note that you appear to have a type error in CI

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 very nice

@Peeja Peeja force-pushed the better-composeMiddleware branch from be569c8 to 1b5fccc Compare January 10, 2025 16:26
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants