kwhitley / itty-router

A little router.
MIT License
1.71k stars 77 forks source link

Golf the regex #140

Closed DrLoopFall closed 1 year ago

DrLoopFall commented 1 year ago

Few tests fails:

× route /. matches path /f

Is there any reason to not escape all dots? I can revert that if needed.

× route /test.:x returns params { x: "a.b" } from path /test.a.b

Now the format params can't contain dots.

× route /x/:y* returns params { y: "test" } from path /x/test

This is same as /x/:y/*, e.g. /x/test, /x/test/something => { y: "test" }. Route with base: /:foo and route: * should work fine.

Fixes:

/foo:bar? matching /foXbar ({ bar: "Xbar" })

Need to write test for this.

DrLoopFall commented 1 year ago

I think we should escape all regex characters, and only allow (, ) and |, that would be enough for most of the cases.

This should do, source MDN

.replace(/[.+?^${}[\\]/g, '\\$&')

edit: Maybe this is not a good idea. We should just document valid/invalid characters.

kwhitley commented 1 year ago

Taking a look at this now!

kwhitley commented 1 year ago

A couple thoughts (I'll edit this as I progress):

kwhitley commented 1 year ago

× route /x/:y* returns params { y: "test" } from path /x/test

This is same as /x/:y/*, e.g. /x/test, /x/test/something => { y: "test" }. Route with base: /:foo and route: * should work fine.

Yeah I'm fine with that. I think the existing test is a behavior that should be discouraged anyway, as it can get folks in trouble.

kwhitley commented 1 year ago

× route /test.:x returns params { x: "a.b" } from path /test.a.b

Now the format params can't contain dots.

We should discuss this on Discord, but I think I'm fine with this. The most important thing I think is to make sure the filename/prefix can contain dots without issue. This appears preserved, as the previous test still passes:

 ✓ route "/:x.:y" returns params { x: "a.b", y: "c" } from path "/a.b.c"
kwhitley commented 1 year ago

I think we should escape all regex characters, and only allow (, ) and |, that would be enough for most of the cases.

This should do, source MDN

.replace(/[.+?^${}[\\]/g, '\\$&')

edit: Maybe this is not a good idea. We should just document valid/invalid characters.

I agree with your last - that we should figure out and document what works/doesn't. Obviously since it's regex generating regex, there's a bit that gets lost-in-translation and simply doesn't work. But some current regex chars/patterns do work, through a happy little accident... and I know some folks are already relying on those things working.

DrLoopFall commented 1 year ago

Currently, the format parameter is only supported after normal parameter, e.g. /:file.:ext. In patterns like /test.:x, the x is considered as normal parameter (may contain dots), so it does matches the path /test.a.b.

This PR allows the format parameter to be anywhere in the pattern, so x is considered as format parameter and won't match dots. To use normal parameter with leading dot, it should be escaped, something like /test[.]:x or /test(.):x.

kwhitley commented 1 year ago

A. I freaking LOVE this.

Sorry it's taken so long to get to this - I got caught up in life and then the v4.x overhaul of the library!

So I've tested your regex in the v4.x, and the following tests break: image

I'm perfectly fine with each of these failures - as they were more documenting weird non-ideal patterns than anything else. I totally agree with an extension formatter that does not pick up dots.

While I can definitely blend in your regex, modify the existing tests to work, and shower you with credit in the CHANGELOG/README, I'd certainly love for you to have line credit for this awesome work. If you do too, I'd just ask that you reform the PR against the v4.x branch, and modify/remove those offending tests. I'll also want one or two that specifically confirm the expected behavior:

/file.:ext // matches /file.jpg but not /file.jpg.bak
/:file.:ext // matches /file.jpg.bak as { file: 'file.jpg', ext: 'bak' }
DrLoopFall commented 1 year ago

Thank you, merged v4.x.

kwhitley commented 1 year ago

Super excited for this one, thanks!!!