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

Should there be a way to set a default route? (not found) #13

Closed
DoumanAsh opened this issue Jan 13, 2017 · 10 comments
Closed

Should there be a way to set a default route? (not found) #13

DoumanAsh opened this issue Jan 13, 2017 · 10 comments

Comments

@DoumanAsh
Copy link
Contributor

While looking into code i found that regardless if route is found or not, next middleware is called. So you cannot exactly be sure without examining koa context wether route is matched or not.

What do you think about adding possibility of configuring default router(i.e. 404 router)?

@tunnckoCore
Copy link
Owner

tunnckoCore commented Jan 13, 2017

meeh.. maybe good point. i'll look at that in one two days.

it is more correct to be said 404 route, not router. but yea i understand.

thanks

@DoumanAsh DoumanAsh changed the title Should there be a way to set a default router? (not found) Should there be a way to set a default route? (not found) Jan 13, 2017
@DoumanAsh
Copy link
Contributor Author

I can propose PR with maybe new method defaultRoute which basically will just add route to be called before passing to next middleware https://github.com/tunnckoCore/koa-better-router/blob/master/index.js#L548

The most simple and straightforward way, i suppose :)

@tunnckoCore
Copy link
Owner

I'm not sure. Or at least I don't see sense to add defaultRoute or such method.

Don't know, let me think it. :)

@tunnckoCore
Copy link
Owner

Koa by default send 404 if nothing happen. As about should it call the next middleware if route not found... i'm still not sure. I believe I made it the same way as koa-router (for example) do it, but I'll recheck.

@DoumanAsh
Copy link
Contributor Author

DoumanAsh commented Jan 19, 2017

The idea is to let your user to decide what to do if it cannot hit any route.
As of now user is forced to check for ctx.route to determine wether router found match or not.
Btw, i think it isn't even documented :)

E.g. https://github.com/DoumanAsh/lazy-http-can/blob/master/server/index.js#L22-L29

It would be good to let user customize 404 response without need to add own middleware.

As about should it call the next middleware if route not found...

It should and it is doing so right now, no need to change it :)
After all user might want to have some post-processing of final response.

@tunnckoCore
Copy link
Owner

tunnckoCore commented Jan 20, 2017

Mmm. Maybe we can add this.options.notFound(ctx) before the return next() on L548? Could you try it locally and report.

or just:

return typeof this.options.notFound === 'function'
  ? this.options.notFound(ctx, next)
  : next()

@DoumanAsh
Copy link
Contributor Author

DoumanAsh commented Jan 20, 2017

I'll try but isn't this.options passed to pathMatch?
Would it be safe to mix options like that?

UPD:
Replaced my default router with your proposal and it works.
Still i'm not sure it is good idea to mix options of path-match and koa-better-router like that

Code

return this.options.not_found ? this.options.not_found(ctx, next) : next()

@tunnckoCore
Copy link
Owner

Yea, they are passed to path-match and so to path-to-regexp, but it is safe looking at the path-to-regexp code.

Still i'm not sure it is good idea to mix options

worse idea would be to allow passing options to .middleware method, I tried in first releases and removed it with reason - too many entry points, too much flexibility :D

Code

self preference is camelCase, so PR is is welcome for that line. But please use the commit convention.

@tunnckoCore
Copy link
Owner

@DoumanAsh just published 2.1 :)

@DoumanAsh
Copy link
Contributor Author

Thanks, wanted to do it myself, but i got a bit busy :)

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

No branches or pull requests

2 participants