lukeed / trouter

:fish: A fast, small-but-mighty, familiar fish...errr, router*
MIT License
634 stars 23 forks source link

Incorrect params parsing? #8

Closed mhkeller closed 5 years ago

mhkeller commented 5 years ago

With a route like the following, I expect the last :y parameter to not include .mvt but it's currently reading :y.mvt as my param key and including the string .mvt in the param value.

polka()
  .get('/:z/:x/:y.mvt', async (req, res) => {
    const { params } = req;
    console.log(params.y) // undefined
    console.log(params['y.mvt'] // 'myY.mvt'

    // workaround
    const y = params['y.mvt'].replace('.mvt', '');
    console.log(y) // 'myY'

I'm porting an app over from express that parsed this format as expected. But maybe this URL structure is no good to begin with!

lukeed commented 5 years ago

Hey, this is expected... for now. The matchit library looks at everything in between path separators, and from there decides if the segment is a static path, a named parameter, an optional parameter, or a wildcard. It's (currently) taking anything in those segments, as is, for each type.

I can change is so that y is the correctly the parameter name, but in the routing/matching phase, it wouldn't ensure that .mvt is part of the path. AKA, it would match .../myY as well as ../myY.mvt.

As I'm sure you've seen, that's also currently the case. The difference would be that I can get you params.y although I don't think there's much point without the route-filter counterpart.

mhkeller commented 5 years ago

Ya makes sense. I don't have deep opinions on how it should operate since I imagine there's a great variety of URL formats of which I am blissfully unaware. If you want to keep this open to have others possibly discuss more desired behavior feel free. My use case is small enough that I'm fine with my workaround, just wanted to raise it in case it was a bug.

lukeed commented 5 years ago

Thanks.

I have a solution here which allows for parsing/matching against any extension-like parameters. This will also apply route filtering.

I've not (yet?) published this because it drops 100k to 400k ops/sec in each of the benchmarks. I'll sit on it & see if a better way comes to mind.

lukeed commented 5 years ago

Transferred the issue as Trouter is technically the router.

I did come up with a better solution, which will be released at a later date. For now, the previous matchit solution will suffice.

Congrats on an amazing article @mhkeller 👏

lukeed commented 5 years ago

Technically closed via matchit@1.0.7 but leaving this open until I merge the solution I'm happier with.

mhkeller commented 5 years ago

Ah thanks! It was a fun one to work on. Very cool. Thanks for continuing to commit the brain cells to this one.

lukeed commented 5 years ago

FInally~