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(middleware/combine): Introduce combine middleware #2941

Merged
merged 3 commits into from
Jul 13, 2024

Conversation

usualoma
Copy link
Member

@usualoma usualoma commented Jun 9, 2024

Proposes middleware to compose multiple middleware. This middleware provides the following three functions.

  • some : Create a composed middleware that runs the first middleware that returns true.
  • every : Create a composed middleware that runs all middleware and throws an error if any of them fail.
  • except : Create a composed middleware that runs all middleware except when the condition is met.

Usage is as indicated in comments and tests.

This change will allow users to write code similar to that illustrated in the comments below.

#2813 (comment)

import { Hono } from 'hono'
import { ipLimit } from 'hono/ip-limit'
import { bearerAuth } from 'hono/bearer-auth'
import { rateLimit } from '@/my-rate-limit'
import { some, every } from 'hono/predicate' // New middleware
import { getConnInfo } from 'hono/...'

const app = new Hono()

// Very complex access restriction rules.
app.use(
  '*',
  some([
    every([
      ipLimit(getConnInfo, { allow: [ '192.168.0.2' ] },
      bearerAuth({ token })
    ]), // If both are satisfied, rateLimit is not necessary.
    rateLimit()
  ])
)
app.get('/', (c) => c.text('Hello world!'))

Name of middleware

I named it "predicate", but I am not particularly particular about it, so it could be named something else. I also thought of "compose", but it is now "predicate" because it is confusing because it is covered by "compose", which we use internally.

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code
  • Add TSDoc/JSDoc to document the code

Copy link

codecov bot commented Jun 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.91%. Comparing base (fe7cfcf) to head (af3e7c5).
Report is 60 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2941      +/-   ##
==========================================
+ Coverage   94.42%   95.91%   +1.49%     
==========================================
  Files         136      138       +2     
  Lines       13343    13662     +319     
  Branches     2219     2299      +80     
==========================================
+ Hits        12599    13104     +505     
+ Misses        744      558     -186     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yusukebe
Copy link
Member

Hey @usualoma .

This middleware is unique and exciting, and implementations like except are very interesting. I think there are other use cases like this where we want to control middleware conditions, so let me think about that for a while!

@yusukebe
Copy link
Member

yusukebe commented Jul 6, 2024

Hi @usualoma

Sorry for the super late reply. My concern is only the naming! Honestly, I don't like a predicate. I also think the compose is best, but as you said, we already have the compose.ts By the way, I've asked ChatGPT:

CleanShot 2024-07-06 at 17 24 44@2x

CleanShot 2024-07-06 at 17 27 40@2x

Hmm. I prefer to arrange or aggregate. Do you have any good other names?

@usualoma
Copy link
Member Author

usualoma commented Jul 8, 2024

@yusukebe Thank you!

I see.
This middleware creates 'composite middleware' in the same way as 'composite function', so I think composite-middleware might be possible if the name is long enough. This name is only written at import time, so it is not a concern if it is somewhat long.

import { some, every } from 'hono/composite-middleware'

But I agree with you on arrange.

Do you want to arrange?

@EdamAme-x
Copy link
Contributor

I think compose is the best choice.

I understand the duplication with an already existing function (compose.ts), but that function is a private function within Hono.
So from the user's point of view, there should be no problem.

import { some, every } from 'hono/compose'

@EdamAme-x
Copy link
Contributor

My idea

import { some, every } from 'hono/grouping'

but, duplication with .route

@yusukebe
Copy link
Member

Hey @usualoma @EdamAme-x !

I'm considering it. Your suggestions are good, but:

  • hono/composite-middleware is long.
  • hono/compose and hono/grouping: users may imagine this as another feature.

And arrange seemed good, but that word also means "to plan, prepare, or organize something".

Then. What about combine?

import { Hono } from 'hono'
import { every, some } from 'hono/combine'

@EdamAme-x
Copy link
Contributor

EdamAme-x commented Jul 12, 2024

hi @yusukebe
How about this?
hono/join

join is simple and clear.

@yusukebe
Copy link
Member

@EdamAme-x

I prefer combine to join. The meaning of combine is better. According to ChatGPT:

In summary, "combine" is about mixing things to create something new, while "join" is about linking things together.

For example, I think it's weird that hono/join exports except. Plus, from the context of Array.prototype.join(), it will just link, but this middleware does more complex things. And combine is enough to be simple and clean.

@EdamAme-x
Copy link
Contributor

combine is short, simple, and easy to remember!

I prefer combine to join. The meaning of combine is better. According to ChatGPT:

@usualoma
Copy link
Member Author

OK, let's proceed with combine.

@usualoma usualoma changed the title feat(middleware/predicate): Introduce predicate middleware feat(middleware/conbine): Introduce combine middleware Jul 12, 2024
@usualoma usualoma changed the title feat(middleware/conbine): Introduce combine middleware feat(middleware/combine): Introduce combine middleware Jul 12, 2024
@yusukebe
Copy link
Member

@usualoma Thanks!

@usualoma
Copy link
Member Author

Updated af3e7c5
My work is completed.

Dependency on TrieRouter

There is no functional problem, but I am a little concerned about whether it is acceptable to have a dependency on TrieRouter.

https://github.com/honojs/hono/pull/2941/files#diff-0127ebff640ea2834e4434792875961100daac21bcdaae2d033a6a59815da3bfR132

It would be best if I could use the router I use for that app. But there's no way to get that, so it's not an option at the moment, though.

@yusukebe yusukebe added the v4.5 label Jul 13, 2024
@yusukebe
Copy link
Member

@usualoma

There is no functional problem, but I am a little concerned about whether it is acceptable to have a dependency on TrieRouter.

I have considered it for a while and concluded that using TrieRouter is good, as is yours. As you said, it would be best if it could use the current app's router, but using TrieRouter is fine. Many users use the default SmartRouter, which includes TrieRouter.

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe yusukebe changed the base branch from main to next July 13, 2024 09:47
@yusukebe
Copy link
Member

Hi @usualoma

I've reviewed it. There is no problem, and it looks good! Thank you for your awesome work. I'll merge this into the next branch.

@yusukebe yusukebe merged commit 9a6e52d into honojs:next Jul 13, 2024
14 checks passed
@usualoma usualoma deleted the feat/predicate-middleware branch July 13, 2024 10:36
@usualoma
Copy link
Member Author

@yusukebe Thank you!

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.

None yet

3 participants