lukeed / polka

A micro web server so fast, it'll make you dance! :dancers:
MIT License
5.39k stars 172 forks source link

Polka does not seem to use `onNoMatch` if route parameters not provided #143

Closed rschristian closed 4 years ago

rschristian commented 4 years ago

Do you want to request a feature or report a bug?

I believe this is a bug.

What is the current behaviour?

I'm building a simple API where one route retrieves some user data, any other route should direct to a 404/instruction page. Currently my setup looks something like this:

polka({ onNoMatch: sirv('assets') })
    .get('/user/:username', async (req, res) => {
        ...
    })
    .listen(PORT, (err) => {
        if (err) throw err;
        console.log(`> Running on localhost:${PORT}`);
    });

The onNoMatch handler will correctly fire for every route excluding /user and /user/x, with /user never being resolved.

What is the expected behaviour?

What I'd expect to happen is that /user would fallback to onNoMatch. /user does not match /user/:username but for some reason onNoMatch can't pick it up.

There are a few workarounds, namely making the route parameter optional and handling it myself, or adding a "catch-all" route for this situation:

polka({ onNoMatch: sirv('assets') })
    .get('/user/:username', async (req, res) => {
        ...
    })
    .get('*', async (req, res) => {
        ...
    })
    .listen(PORT, (err) => {
        if (err) throw err;
        console.log(`> Running on localhost:${PORT}`);
    });

Please mention other relevant information.

Currently using v0.5.2, but the problem persists in v1.0.0-next.11.

Edit: Though now as I continue to play around I can't get the onNoMatch to fire for anything other than /... Not sure if I broke something or it wasn't correctly working to begin with.

rschristian commented 4 years ago

I totally misunderstood what sirv and the like were for. My mistake.

lukeed commented 4 years ago

No problem! :D

For the record, here's how you'd do this with both versions:

polka()
  .use( sirv('assets') )
  .get('/user/:username', async (req, res) => {
        ...
    })
  .listen()

If you wanted to optimize it a little bit with polka@next, you could do this too:

polka()
  .get('/user/:username', async (req, res) => {
        ...
    })
  .use( sirv('assets') )
  .listen()

This is an "optimization" only if you have a few API routes and want to check those before searching for an asset to send.

Hope that helps!

rschristian commented 4 years ago

Yeah, totally get that now.

I had made the (admittedly rather silly in hindsight) assumption that sirv, as I had it set up, would respond with the index.html file I had sat in /assets/ all of the time. Of course, this isn't the case, but that's why I was trying to use the onNoMatch option. As I had no other static assets I didn't realize the full extent of what sirv was actually doing for me here.

Simple solution was to just add a redirect in the "catch-all" I mentioned to /. Works like a charm.

But thanks for the tip, will probably use it. Cheers!