Processed handler should be only one

yusukebe commented 2 years ago

In the Iusse #189 , we decided to treat handlers and middleware as the same. We can write like these:

app.post('/a', basicAuth(), bodyParse(), (c) => c.text('a)')

This is good.

How about Handler + Handler pattern.

app.get('/a', (c) => {
  console.log('a')
  return c.text('a')
})

app.get('/:slug', (c) => {
  console.log('slug')
  return c.text('slug')
})

This Response body will be a. This is expected.

BUT, in #258 I notice that every handler is processed. console.log emits slug, a.

The reason:

All routes including middleware are treated as the same.
A path will match multiple routes = handlers or middleware.
If multiple handlers are matched, every handler will be invoked.
Route returning Response object is determined as the handler.
We don't know whether the route is a handler or Middleware until dispatching 😕

I think this design is not so good. Only one handler should be invoked. In this case, console.log should emit only a. So, we have to:

Determine handler or middleware.
Make only one handler invoked.

How can we do that?

yusukebe commented 2 years ago

How about this:

Basically, to make the route as a handler, use c.HTTP_METHOD.

app.get(path, handler)

---> app.get('/a', (c) => c.text('a))

To make the route as middleware, use c.use.

app.use(path, middleware)

 ---> app.use('/a', cors())

If we want to specify HTTP method for applying middleware, use c.HTTP_METHOD.

app.get('/a', middleware, middleware.., handler)

 ---> app.post('/a', basicAuth(), bodyParse(), (c) => c.text('a)')

The order is important. Last route will be handler, others will be middlware.

usualoma commented 2 years ago

Hi! @yusukebe

Just to be sure, are you saying that we should still have considered the matter I am talking about in this comment?

https://github.com/honojs/hono/issues/189#issuecomment-1121693657

I agree with the specifications you wrote above. This was exactly what I wanted to suggest in my comments below. Sorry if I did not communicate it well.

https://github.com/honojs/hono/issues/189#issuecomment-1120625620

The implementation would be as follows: "If multiple handlers are detected, only the last one will be executed." (It would be best if it could be handled within router class, as it would be costly to do so with every dispatch call.)

diff --git a/src/hono.test.ts b/src/hono.test.ts
index c241140..80f70cd 100644
--- a/src/hono.test.ts
+++ b/src/hono.test.ts
@@ -432,7 +432,7 @@ describe('Middleware with app.HTTP_METHOD', () => {
   describe('Basic', () => {
     const app = new Hono()

-    app.all('*', async (c, next) => {
+    app.use('*', async (c, next) => {
       await next()
       c.header('x-custom-message', 'hello')
     })
diff --git a/src/hono.ts b/src/hono.ts
index 62e5a10..230b5d9 100644
--- a/src/hono.ts
+++ b/src/hono.ts
@@ -31,10 +31,14 @@ declare global {
   }
 }

-export type Handler<RequestParamKeyType extends string = string, E = Env> = (
-  c: Context<RequestParamKeyType, E>,
-  next: Next
-) => Response | Promise<Response> | void | Promise<void>
+export type Handler<RequestParamKeyType extends string = string, E = Env> = {
+  (c: Context<RequestParamKeyType, E>, next: Next):
+    | Response
+    | Promise<Response>
+    | void
+    | Promise<void>
+  isHandler?: boolean
+}
 export type NotFoundHandler<E = Env> = (c: Context<string, E>) => Response
 export type ErrorHandler<E = Env> = (err: Error, c: Context<string, E>) => Response
 export type Next = () => Promise<void>
@@ -101,15 +105,17 @@ export class Hono<E = Env, P extends string = '/'> extends defineDynamicClass()<
     allMethods.map((method) => {
       this[method] = <Path extends string>(
         args1: Path | Handler<ParamKeys<Path>, E>,
-        ...args: [Handler<ParamKeys<Path>, E>]
+        ...args: Handler<ParamKeys<Path>, E>[]
       ) => {
         if (typeof args1 === 'string') {
           this.path = args1
         } else {
+          args1.isHandler = args.length === 0
           this.addRoute(method, this.path, args1)
         }
-        args.map((handler) => {
+        args.map((handler, i) => {
           if (typeof handler !== 'string') {
+            handler.isHandler = i === args.length - 1
             this.addRoute(method, this.path, handler)
           }
         })
@@ -202,6 +208,18 @@ export class Hono<E = Env, P extends string = '/'> extends defineDynamicClass()<
       }
     }) as typeof request.param
     const handlers = result ? result.handlers : [this.notFoundHandler]
+    if (result) {
+      for (let i = handlers.length - 1, hasHandler = false; i >= 0; i--) {
+        if (!hasHandler) {
+          hasHandler = (handlers[i] as Handler).isHandler
+          continue
+        }
+
+        if ((handlers[i] as Handler).isHandler) {
+          handlers.splice(i, 1)
+        }
+      }
+    }

     const c = new Context<string, E>(request, {
       env: env,

yusukebe commented 2 years ago

Hi @usualoma

Just to be sure, are you saying that we should still have considered the matter I am talking about in this comment?

Yes. I'm sorry too for not understanding enough.

I agree with the specifications you wrote above.

Thank you!

The implementation would be as follows: "If multiple handlers are detected, only the last one will be executed." (It would be best if it could be handled within router class, as it would be costly to do so with every dispatch call.)

I like your implementation. It's simple and covers all patterns. I think we don't have to rewrite routers. Let’s go with it!

Could you create the pull request for this code?

metrue commented 2 years ago

Determine handler or middleware.

To be honest, I think there should be there're (or should be) no differences between hander and middleware, or we can say the handler is just a response middleware. Both of them are async functions, compose function make a process pipeline of them for the incoming fetch event, each unit (middleware) does a simple operation on the context (req, headers, res), and response a result to the client, the whole process is simple and beautiful. To determine a function is handler or middleware, requires us to put extra information onto the functions, which break the simplicity but without obvious benefit.

Make only one handler invoked.

When functions are registered for a path, I think they should be all invoked by order since that's what the code designer want.

The issue you guys mentioned in https://github.com/honojs/hono/issues/258, I think it's not about middleware or handler stuff, it's about path matching algorithm.

app.get('/:slug', m2(), m3());
app.get('/a', m0(), m1());

it's about how we think about /a and /:slug, since we think /a is a special case for /:slug, then [m0, m1] is for /a, and [m2, m3] is for /:slug.

usualoma commented 2 years ago

@metrue Thanks!

I had overlooked that case.

app.get('/:slug', m2(), m3());
app.get('/a', m0(), m1());

I agree with the specification that the routing results should be as follows

[m0, m1] is for /a, and [m2, m3] is for /:slug.

The patch above is not good enough for that.

Distinguish between a middleware or a handler

However, I still think that internally we have to distinguish between a middleware or a handler.

app.use('/posts/*', a); // a is a middleware
app.get('/posts/*', b); // b is a handler
app.get('/posts/a', c); // c is a handler

I don't think we are compromising the simplicity of compose.ts here. I think the information about whether it is a handler or middleware can be treated as information for routing purposes.

I think the following implementation could solve the problem. (there is much room for performance improvement)

https://github.com/honojs/hono/compare/master...usualoma:ensure-one-handler-2

metrue commented 2 years ago

@usualoma You codes look nice, but I would like to share a little more about this middleware&handler concept.

I know we're doing differently on router (I mean the router object: router.ts) with Koa and Express, they make router just a middleware. But the design philosophy is worth learning: An Express/Koa application is essentially a series of middleware function calls, with this fundamental concept, things get easy: users register the functions for endpoints one by one which they wish to be invoked in order for a incoming request; The frameworks get the incoming request, compose the context, invoke all the functions and response context's res.

Let's take a look on your example,

app.use('/posts/*', a); 
app.get('/posts/*', b); 
app.get('/posts/a', c);

Above codes actually tells the route matching attention:

Function a should be invoked for path /post/* on all HTTP methods.
Override the process pipeline of GET /posts/* with a process pipeline with only on function b.
Override the process pipeline of GET /post/a' with a process pipeline with only on functionb`.

So we can see a, b, and c are just functions without any differences on matching and invoking level,

yusukebe commented 2 years ago

Hi @metrue @usualoma !

from @metrue comment: there should be there're (or should be) no differences between hander and middleware

I really think so. We want to write not only using c.use but like this:

app.get('/', md1) // <--- dispatch middleware GET /

I think this issue is just about implementation.

Sorry to go back to issue #189. I've noticed we have a simple way to solve this issue. Use Routing Order Specification proposed @usualoma https://github.com/honojs/hono/issues/189#issuecomment-1121631592

Koa/Express is using this way too.

If we remove this one line from current code:

diff --git a/src/compose.ts b/src/compose.ts
index fb49e13..c35bf45 100644
--- a/src/compose.ts
+++ b/src/compose.ts
@@ -32,7 +32,6 @@ export const compose = <C>(
           if (res && context instanceof Context) {
             context.res = res
             context.res.finalized = true
-            await dispatch(i + 1) // <--- Call next()
           }
           return context
         })

and, run the tests below:

    const handler1 = (c: Context) => {
      console.log('handler1')
      return c.text('handler1')
    }

    const handler2 = (c: Context) => {
      console.log('handler2') // <--- this log will no be emmited
      return c.text('handler2')
    }

    const composed = compose([md1, md2, handler1, handler2])
    await composed(c)
    expect(await c.res.text()).toBe('handler1')

handler1 will be dispatched but, handler2 will not be dispatched because

handler1 is dispatched before handler2.
When the handler returns Response object, it stops dispatching

This behavior is expected.

But, by current implementation, this tests are failed:

    app.get('/posts/:id', (c) => {
      return c.text('first')
    })

    app.get('/:type/:id', (c) => {
      return c.text('second')
    })

    it('Should return response from `specialized` route', async () => {
      const res = await app.request('http://localhost/posts/123')
      expect(await res.text()).toBe('first') // <--- Failed!!! to be 'second'
    })

Because sort order depends on the router's behavior, it is decided by how it is specialized.

If use Routing Order Specification by @usualoma :

Sort by number of slashes
Sort by registration order if the number of slashes in the same

it will be more simple. If we want to dispatch handler a, simply put a above.

Then, we can write like a Koa/Express, @metrue said.

I'm really sorry to go back to the story, but I think this is a good way.

usualoma commented 2 years ago

Koa/Express is using this way too.

Um, I think..., in the Express, the call order depends only on the order of registration, and the content of the path has no effect on the call order at all. (I have not tested in the Koa.) If we call the functions in the order of registration and stop processing the compose function when the Response object is returned, I think we will get the same behavior.

for http://localhost/a ...

app.use('/posts/*', function a(c, next) { next() }); 
app.get('/posts/*', function b(c, next) { next() }); 
app.get('/posts/:slug', function c(c) { return c.text('slug') }); 
app.get('/posts/a', function d(c) => { return c.text('a') }); 
app.get('/*', function e(c, next) { next() }); 
=> a => b => c (d, e should not be called)

app.use('/posts/*', function a(c, next) { next() }); 
app.get('/posts/*', function b(c, next) { next() }); 
app.get('/posts/a', function d(c) => { return c.text('a') }); 
app.get('/posts/:slug', function c(c) { return c.text('slug') }); 
app.get('/*', function e(c, next) { next() }); 
=> a => b => d (c, e should not be called)

app.get('/posts/a', function d(c) => { return c.text('a') }); 
app.get('/posts/:slug', function c(c) { return c.text('slug') });
app.use('/posts/*', function a(c, next) { next() }); 
app.get('/posts/*', function b(c, next) { next() });  
app.get('/*', function e(c, next) { next() }); 
=> d  (a, b, e and c should not be called)

We might want to look back at this Express specification as well and consider whether the https://github.com/honojs/hono/issues/189#issuecomment-1121631592 specification is what we want.

I tested with express 4.18.1.

const express = require('express')
const app = express()
const port = 3000

app.use('/posts/*', (req, res, next) => {
  console.log('middleware');
  next();
})

app.get('/posts/:slug', (req, res, next) => {
  console.log('slug');
  next();
})

app.get('/posts/abc', (req, res, next) => {
  console.log('abc');
  next();
})

app.get('/*', (req, res) => {
  res.send('content')
})

app.listen(port, () => {
  console.log(`Example app listening on port ${port}`)
});

yusukebe commented 2 years ago

Hmm, that's right.

To say simply, we want to do this: right?

app.get('/*', async (_c, next) => { await next()}) // <--- a
app.get('/:slug', (c) => { return c.text('/:slug') }) // <--- b
app.get('/page', (c) => { return c.text('/page') }) // <--- c

GET /page ---> a => c
GET /123 ---> a => b

metrue commented 2 years ago

app.get('/*', async (_c, next) => { await next()}) // <--- a
app.get('/:slug', (c) => { return c.text('/:slug') }) // <--- b
app.get('/page', (c) => { return c.text('/page') }) // <--- c 

GET /page ---> a => c
GET /123 ---> a => b

I have different expectation, I think it should be

GET /page ---> c
GET /123 ---> b

GET /xxx  ---> a invoked, but res is initial value since it not set in any middleware

Based on the ideas:

.use (or .get, .post, .delete, etc) register the middlewares one by one in order, invoking order is same with registering order.
Special endpoint (which is made of HTTP method and path) has a high priority in middleware register, it will override the general pattern endpoint during middleware registration.

yusukebe commented 2 years ago

@metrue Hi

If you want to dispatch logger in every path, how can you write? I want to write like this:

app.get('/*', logger()) // <--- logger
app.get('/:slug', (c) => { return c.text('/:slug') }) // <--- b
app.get('/page', (c) => { return c.text('/page') }) // <--- c

GET /page ---> logger + c
GET /123 ---> logger + b

metrue commented 2 years ago

@yusukebe

It would be,

app.use(logger())

Since no path and method specified, the logger() middleware will be pushed into global middleware queue, which means all the incoming fetch event will go through it.

app.get('/:slug', (c) => { return c.text('/:slug') }) // <--- b

Register b middleware for GET /:slug, its middlewares list become [logger(), b()]

[logger(), b()] = [global middlewares] + [endpoint middlewares]

app.get('/page', (c) => { return c.text('/page') }) // <--- c

Register c for specific endpoint GET /page, its middlwares list became [logger(), c()]

[logger(), c()] = [global middlewares] + [specific endpoint middlewares]

usualoma commented 2 years ago

How about something like this? https://github.com/honojs/hono/compare/master...usualoma:update-router-interface

Design

Sort routes based on path strings according to certain rules.
- "registration order", or " #189 (comment) specification", or some other user defined rule
If a Response object is returned from a handler, do not process the rest.

Sorting Rules

In the branch above, the default is the same rules as the Express. This is not because this is preferred, but because it is easier to implement.

  // sort by https://github.com/honojs/hono/issues/189#issuecomment-1121631592
  const app = new Hono({
    router: new TrieRouter({
      scorer: (() => {
        let index = 0
        return (path) => `${(path.match(/\/(?::\w+{[^}]+}|[^\/]*)/g) || []).length}.${index++}`
      })(),
    }),
  })

  app.get('*', function a() {})
  app.get('*', function b() {})
  app.get('/*', function c() {})
  app.get('/api', function d() {})
  app.get('/api/:type/:id', function e() {})
  app.get('/api/posts/:id', function f() {})
  app.get('/*/*/:id', function g() {})

  app.request('http://localhost/api/posts/1')

  // a => b => c => e => f => g

  // sort by level of detail
  const app = new Hono({
    router: new TrieRouter({
      scorer: (path) => {
        if (path === '*' || path === '/*') {
          return '0'
        }

        const components = path.match(/\/(?::\w+{[^}]+}|[^\/]*)/g) || []
        const paramIndexList: number[] = []
        for (let i = 0, len = components.length; i < len; i++) {
          if (/\/(?::|\*)/.test(components[i])) {
            paramIndexList.push(i)
          }
        }
        return [components.length]
          .concat(paramIndexList)
          .map((i) => String(999 - i).padStart(3, '0'))
          .join('')
      },
    }),
  })

  app.get('*', function a() {})
  app.get('*', function b() {})
  app.get('/*', function c() {})
  app.get('/api', function d() {})
  app.get('/api/:type/:id', function e() {})
  app.get('/api/posts/:id', function f() {})
  app.get('/*/*/:id', function g() {})

  app.request('http://localhost/api/posts/1')

  // a => b => c => f => e => g

Matter of concern

Sorting is slow, so it is not desirable to require sorting at request time
- This problem can be solved by pre-sorting the RegExpRouter, but may be a bit more difficult with the TrieRouter.

yusukebe commented 2 years ago

Oh, I'm thinking the same thing, and writing the code for this feature 😆 https://github.com/honojs/hono/compare/master...router-ordering

In my implementation, the sorting rule is like below / maybe almost are as same as the code by @usualoma

describe('Complex', () => {
  const node = new Node()
  node.insert('get', '/api/*', 'c') // score 2.1
  node.insert('get', '/api/:type/:id', 'd') // score 3.2
  node.insert('get', '/api/posts/:id', 'e') // score 3.3
  node.insert('get', '/api/posts/123', 'f') // score 3.4
  node.insert('get', '/*/*/:id', 'g') // score 3.5
  node.insert('get', '/api/posts/*/comment', 'h') // score 4.6 - not match
  node.insert('get', '*', 'a') // score 1.7
  node.insert('get', '*', 'b') // score 1.8

  it('get /api/posts/123', async () => {
    const res = node.search('get', '/api/posts/123')
    // ---> will match => c, d, e, f, b, a, b
    // ---> sort by score => a, b, c, d, e, f, g
    expect(res.handlers).toEqual(['a', 'b', 'c', 'd', 'e', 'f', 'g'])
  })
})

I agree with your opinion!

usualoma commented 2 years ago

I have confirmed https://github.com/honojs/hono/compare/master...router-ordering!

It was simpler than mine and I felt it was implemented well enough for we needs. There was also a test. (There was no test in my branch) I hope we can proceed based on your branch.

yusukebe commented 2 years ago

@usualoma Thank you for confirming.

Hi @metrue

I want to go with this. The koa/express design might be nice, but I like this one. And it's not so different. I'll try to summarize it.

Definition

Handler and Middleware are almost the same. The differences are below:

Handler: return Response object.
Middleware: return nothing, manipulate or look Request/Response after/before dispatching, processed withnext()

Middleware

The user can register middlewares using c.use or register these using c.HTTP_METHOD as well as the handlers. For this feature, it's easy to specify the path and the method.

// match any method, all routes
app.use('*', logger())

// specify path
app.use('/posts/*', cors())

// specify method and path
app.post('/posts/*', basicAuth(), bodyParse())

Handler

If the handler returns Response, it will be used for the end-user, stopping the processing.

app.post('/posts', (c) => c.text('Created!', 201))

In this case, four middlewares are processed before dispatching like this:

logger() -> cors() -> basicAuth() -> bodyParse() -> *handler*

Ordering

Matching priority is decided by ordering. The handler that is registered above will be dispatched.

app.get('/book/a', (c) => c.text('a')) // a
app.get('/book/:slug', (c) => c.text('common')) // common

GET /book/a ---> `a` // common will not be dispatched
GET /book/b ---> `common` // a will not be dispatched

All scoring rules:

app.get('/api/*', 'c') // score 2.1
app.get('/api/:type/:id', 'd') // score 3.2
app.get('/api/posts/:id', 'e') // score 3.3
app.get('/api/posts/123', 'f') // score 3.4
app.get('/*/*/:id', 'g') // score 3.5
app.get('/api/posts/*/comment', 'h') // score 4.6 - not match
app.get('*', 'a') // score 1.7
app.get('*', 'b') // score 1.8

GET /api/posts/123
---> will match => c, d, e, f, b, a, b
---> sort by score => a, b, c, d, e, f, g

That's all! I think you'll like it too. So I want to go with this.

usualoma commented 2 years ago

I think the following additional explanation is needed.

Scores for `/*`

Scores may be dynamically determined in the following cases.

app.get('/api/*', 'auth middleware') // score 1.1 or 2.1 ?
app.get('/api', 'b') // score 1.2
app.get('/api/posts', 'c') // score 2.3
app.get('/api/*', 'fallback') // score 1.4 or 2.4 ?

Scores for routings ending with /* are calculated using the following.

`${Math.min(slash_count_of_request_path, slash_count_of_route_path}.${registration_order}`

The results are as follows.

GET /api
---> 
app.get('/api/*', 'authentication') // score 1.1
app.get('/api', 'b') // score 1.2
app.get('/api/posts', 'c') // score 2.3 - not match
app.get('/api/*', 'fallback') // score 1.4

GET /api/posts
---> 
app.get('/api/*', 'authentication') // score 2.1
app.get('/api', 'b') // score 1.2 - not match
app.get('/api/posts', 'c') // score 2.3
app.get('/api/*', 'fallback') // score 2.4

usualoma commented 2 years ago

@yusukebe

In the router-ordering branch, the following code failed.

describe('Blog - failed', () => {
  const router = new TrieRouter<string>()

  router.add('POST', '/entry', 'post entry')
  router.add('POST', '/entry/*', 'fallback')
  router.add('GET', '/entry/:id', 'get entry') // success if this line is commented out
  it('POST /entry', async () => {
    const res = router.match('POST', '/entry')
    expect(res).not.toBeNull()
    expect(res.handlers).toEqual(['post entry', 'fallback'])
  })
})

I think the order property is being overwritten at the following location. https://github.com/honojs/hono/blob/1fe87ece320e815b31fba328d7fe946b09314098/src/router/trie-router/node.ts#L75

yusukebe commented 2 years ago

Hi @usualoma

I've pushed perfect code into https://github.com/honojs/hono/compare/master...router-ordering Fixed problem you said.

There is another matter. It's a little bit tricky but, we have to implement special wildcard.

In this case, /entry/* tasks priority over /entry. It's expected.

describe('Blog - special Wildcard', () => {
  const router = new TrieRouter<string>()

  router.add('POST', '/entry', 'post entry') // 1.01
  // /entry/* will match /entry*
  router.add('POST', '/entry/*', 'special') // 1.00
  it('POST /entry', async () => {
    const res = router.match('POST', '/entry')
    expect(res.handlers).toEqual(['special', 'post entry'])
  })
})

Because, there is a case to apply middleware both /entry /entry/abc by /entry/*. This is already implemented in the current master branch. https://github.com/honojs/hono/blob/52bca3bd0a3b14cae861d47c47437821768b6686/src/router/trie-router/node.ts#L149

Can you implement it with RegExpRouter?

usualoma commented 2 years ago

@yusukebe I think that it is harder to write fallback routing when the following score is set

  router.add('POST', '/entry', 'post entry') // 1.01
  router.add('POST', '/entry/*', 'special') // 1.00

As for the handling of special wildcard, I think the way shown in https://github.com/honojs/hono/issues/262#issuecomment-1135232294 is better, what do you think?

yusukebe commented 2 years ago

@usualoma

I agree with your scoring rule. I've fixed the code and pushed it! https://github.com/honojs/hono/compare/master...router-ordering

usualoma commented 2 years ago

@yusukebe Thank you, I also implemented the RegExpRouter version in https://github.com/honojs/hono/pull/267.

usualoma commented 2 years ago

As an aside...

About `/*` Scoring

I think we can think of scoring against /* as static, rather than as the dynamic we talked about above.

app.get('/entry/*', function a() {})

This can be taken to mean the following

app.get('/entry', function a() {})
app.get('/entry/{accept all remaining components}', function a'() {})

In this light.

app.get('/api/*',  function a() {})
app.get('/api',  function b() {}) 
app.get('/api/posts',  function c() {})
app.get('/api/*',  function d() {})

This means the following

app.get('/api',  function a() {}) // score 1.1
app.get('/api/{accept all remaining components}',  function a'() {}) // score 2.2
app.get('/api',  function b() {})  // score 1.3
app.get('/api/posts',  function c() {}) // score 2.4
app.get('/api',  function d() {}) // score 1.5
app.get('/api/{accept all remaining components}',  function d'() {}) // score 2.6

GET /api
---> 
a() => b() => d()

GET /api/posts
---> 
a'() => c() => d'()

yusukebe commented 2 years ago

Hi @usualoma

Thank you for the PR, I will merge it from now on and will release v1.4.0 including this feature.

I think we can think of scoring against /* as static, rather than as the dynamic we talked about above.

Yes, I know it. It's expected behavior. That's OK!

metrue commented 2 years ago

@yusukebe

I know codes tell everything but it's also great if we put the whole design into README.md or just comments in hono.ts to explain the whole algorithm, just a nice to have.

I think I like everything what you guys are trying to do, except this one,

If the handler returns Response, it will be used for the end-user, stopping the processing.


hono.get(
    '*',
    (ctx, next) => {
        console.log('a1')
        next()
                console.log('a2')
    },
    (ctx, next) => {
        console.log('b1')
        next()
        console.log('b2')
    },
    (ctx, next) => {
        ctx.json('ok1')
                 console.log('ok1')
        next()
    },
    (ctx) => {
        return ctx.json('ok2')
    },
)

What's the output if we GET / ? is it a1 -> b1 -> ok1 -> b2 -> a2 and the response is ok2?

yusukebe commented 2 years ago

@metrue

What's the output if we GET / ? is it a1 -> b1 -> ok1 -> b2 -> a2 and the response is ok2?

Yes!

metrue commented 2 years ago

@yusukebe Excellent, it's my expectation. Coooool.

dan-lee commented 2 years ago

This just bit me! Can we expect a release soon :)? 🚀

yusukebe commented 2 years ago

Hi @dan-lee

Released just now! https://github.com/honojs/hono/releases/tag/v1.4.0

dan-lee commented 2 years ago

@yusukebe Thank you!

Maybe I misunderstood the point of this issue, I've got the following use case:

test('graphql/catch all', () => {
  const router = new TrieRouter<string>()

  router.add('ALL', '/graphql', 'graphql')
  router.add('ALL', '/*', 'everything else')

  const res = router.match('POST', '/graphql')

  expect(res.handlers).toBe(['graphql'])
})

I expected only /graphql to match and not /*, as it is a fallback/catch-all. Instead, both handlers seem to match:

- Expected  - 0
+ Received  + 1

  Array [
+   "everything else",
    "graphql",
  ]

For me this is unexpected because I am returning a response in /graphql and no other routes should do any work from then on.

yusukebe commented 2 years ago

Hi @dan-lee !

We've released v1.4.1. https://github.com/honojs/hono/releases/tag/v1.4.1

This release includes the feature that you expected. Update routing priority and fixed the issue about GraghQL server middleware. The following code works well. Please check it out!

import { buildSchema } from 'graphql'
import { Hono } from 'hono'
import { graphqlServer } from 'hono/graphql-server'

const app = new Hono()

const schema = buildSchema(`
type Query {
  hello: String
}
`)

const rootValue = {
  hello: () => 'Hello Hono!',
}

app.use(
  '/graphql',
  graphqlServer({
    schema,
    rootValue,
  })
)

app.get('*', (c) => {
  return c.text('fallback')
})

export default app

dan-lee commented 2 years ago

Hey @yusukebe, thanks a lot for your great work! It works perfectly now 🤩

honojs / hono