kwhitley / itty-router

A little router.
MIT License
1.75k stars 78 forks source link

Fix route logic bugs and minifier shadowing bug #18

Closed technoyes closed 3 years ago

technoyes commented 3 years ago

The route filter had three bugs:

1) Checked the wrong array entry to find the method (0 instead of 2) 2) Did not make correct use of || 3) The variable named r was shadowing the request variable after minification

Three bugs in one line, but that is what you would expect from such beautiful minimalistic code where every little character counts :-D

Taking this opportunity to thank you for this absolutely awesome router @kwhitley! 🥇

Just love how you managed to cram so much power into so few bytes. Was a pleasure to dig into the enormous code base and find the bug that was causing weird behaviour for me 😄 - learned a lot!

kwhitley commented 3 years ago

Awww man, what a rookie mistake re. the array index (I reordered the internal storage to avoid a [, route, handlers] call with the wasted comma, and the || fail.

I'm just wondering how the hell this passed the test suite after these bugs...

All in all, GREAT catches! Thank you so much!

Now, if you want to take the same scrutiny to the regex (to pay back those characters you correctly added...) ;)

technoyes commented 3 years ago

Small fix to keep travis happy (hopefully)

technoyes commented 3 years ago

Which regex is that? In the prebuild / minifier or something? And yeah, I was thinking the same thing about the tests to be honest LOL 😆

kwhitley commented 3 years ago

Nah the regex of the route match generation (lines 19-21)... I hate that I had to do that in 3 passes, but it's a nasty little affair... writing regex to generate regex (that will later be used in a .match()) 😆

kwhitley commented 3 years ago

Btw I'm going to merge this and test locally before publishing... I can see the (very correct) changes here, regardless of if Travis can get its act together this morning...

kwhitley commented 3 years ago

actually so you get credit for the line, would you mind adding a commit that uses i[2] instead of x[2]? I just prefer i/item for an iterator if I can't use the single letter [appropriate] one (like r)... was gonna do that on mine but you should def get credit/blame for that ;)

Conversely, we can change the variable/letter that request gets moved to during prebuild...

kwhitley commented 3 years ago

Scratch that, these are too critical to wait on the push... thanks again for the great catch @technoyes!!!! <3

Live in 2.0.4

technoyes commented 3 years ago

Haha, I see what you mean with the regex.

No prob, saw you already fixed the "i" name - yes better name!

Again, thanks for the cool router - and for really being "on the ball"!

Will be updating my package.json and go with 2.0.4 🙇

kwhitley commented 3 years ago

My pleasure! It was a totally fun/albeit useless challenge to "code golf" this, but man... I love doing so much with so little, and it's been incredible to see how freaking much this little beast can do.

I'll actually be releasing some supplemental utils/middleware for use with itty (that I developed for my own work with CF Workers) that further add power. Like... a throwable router that proxies itty (so any code executed is automatically caught, without you having to write try/catch in each of your handlers), util functions for json, cors, etc. But to keep the core lib tiny, they'll either be released alongside in folders like itty-router/middleware (likely), or in a completely separate repo (unlikely).

kwhitley commented 3 years ago

image 👍

kwhitley commented 3 years ago

...and then I fixed that typo too.

technoyes commented 3 years ago

Cool! Love more utils. Doing many of those things by hand at the moment, but would be nice to use a nice tight (and tree-shakeable) re-usable library.

If you aren't in the CF Discord already, hop on. Lots of nice people there (including CF Worker team) doing cool stuff for Workers/DO/Pages. Might want to have a look at https://github.com/aggressivelymeows/cfw-easy-utils for example.