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: support variadic middleware for routes #85

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

swain
Copy link
Contributor

@swain swain commented Oct 31, 2023

Motivation

Many existing LifeOmic services use the "variadic middleware" style when declaring routes:

router.get(
  '/something',
  middleware1,
  middleware2,
  middleware3,
  actualImplementation
);

Currently, it's very diffuclt to adopt one-schema in a service that uses this^ pattern, since it can't be represented in a OneSchemaRouter without significant refactoring.

That changes today! This PR introduces support for this syntax in a non-breaking way, thanks to TypeScript magic:

router
  // This is the preferred path, supported today. Single implementation function. This is preferred + clearer!
  .implement(
    'POST /items',
    async (ctx) => {
      return item;
    }
  )

  // But now, this also works:
  .implement(
    'POST /items',
    async (ctx, next) => {
      await doSomethingElse();
      await next();
    },
    async (ctx) => {
      return item;
    }
  )

  // You can have many middlewares.
  .implement(
    'POST /items',
    middleware1,
    middleware2,
    middleware3,
    // The last function in the list _must_ return the required response.
    async (ctx) => {
      return item;
    }
  )

With this^ change, it will be much easier to adopt one-schema in existing services.

@swain swain marked this pull request as ready for review October 31, 2023 16:46
>,
) {
...middlewares: [
...OneSchemaRouterMiddleware<
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, TIL you can spread a type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this is a rest, not a spread.

Copy link
Contributor

@epeters3 epeters3 left a comment

Choose a reason for hiding this comment

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

Very cool.

z.infer<Schema[Route]['response']>,
R
>,
]
Copy link
Contributor

Choose a reason for hiding this comment

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

So this above type for middlewares will require 0 to many OneSchemaRouterMiddleware parameters, followed by exactly 1 required EndpointImplementation parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@epeters3 Exactly! Previously, I thought this was impossible to model using TS. I was wrong :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Way cool. I think the fact that Typescript is fundamentally a correctness checker and DX tool and not just a necessary part of code compilation like with Java or C, has enabled the Typescript maintainers and community to do some pretty incredible things.

@@ -509,6 +510,105 @@ describe('implementations', () => {
});
});

describe('using middleware', () => {
test('type errors are caught when using middleware', () => {
Copy link
Member

Choose a reason for hiding this comment

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

What is actually making the jest assertions here? Or are we just relying on the @ts-expect-error directives to act like a unit test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, just the ts-expect-error directives. This pattern is used in many tests in this project.

Comment on lines +116 to +118
const mws = middlewares.slice(0, -1) as any[];

const implementation = middlewares.at(-1) as any;
Copy link
Member

Choose a reason for hiding this comment

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

What was the reason for having to cast as any here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

middlewares doesn't narrow enough -- it's typed as roughly Array<Middleware | Implementation>

@swain swain requested a review from aecorredor October 31, 2023 18:56
@swain swain merged commit b5f7dc5 into master Oct 31, 2023
3 checks passed
@swain swain deleted the support-variadic-middleware branch October 31, 2023 19:23
Copy link

🎉 This PR is included in version 5.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants