kwhitley / itty-router

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

Router golfing, with 37% less CPU in handle path. #42

Closed taralx closed 3 years ago

taralx commented 3 years ago

Now with introspection!

taralx commented 3 years ago

Hmm, if I move the routes back into the options object, it's smaller, and one can introspect indirectly:

routes = []
r = new Router({routes})
r.all('*', handler)
console.log(routes)
taralx commented 3 years ago

Prototyping says that saves 2 bytes. Thoughts?

taralx commented 3 years ago

More importantly, removing !== undefined is a very big win. How important is it that we allow falsey things to be returned from handle?

kwhitley commented 3 years ago

Hmm, if I move the routes back into the options object, it's smaller, and one can introspect indirectly:

routes = []
r = new Router({routes})
r.all('*', handler)
console.log(routes)

This would require passing { routes } just to run (breaking change), or only for introspection?

UPDATE: I see what you mean... yeah I think moving it back in makes sense, and may even be useful later (e.g. theoretically preloading routes)

kwhitley commented 3 years ago

Stuck on getting these cases to pass (the regex is hairy):

with route: /:image.:format?

/foo --> { image: "foo" } /foo.bar --> { image: "foo", format: "bar" } /foo.bar.baz --> { image: "foo.bar", format: "baz" }

for the last, I can pretty easily attain: /foo.bar.baz --> { image: "foo", format: "bar.baz" }, but that doesn't coincide with how files are typically named

taralx commented 3 years ago

Are you using non-greedy operators?

kwhitley commented 3 years ago

Are you using non-greedy operators?

No, because if I did, it would terminate the param matches wayyyy too soon.

Testing at https://regexr.com/5u94n with regex /^\/(?<id>[^/.]+).?(?<format>[^/]+)?\/*$/ against /foo/bar/baz.

The premise is, we want to capture the first group (including all but the last period, if multiple), then if a final part exists, capture that (minus the period) as the last group. It's a tall order, and we may be asking too much without adding too muxh additional regex... but I haven't even gotten the right mix yet regardless.

I've played with an assortment of tactics, including non-capturing groups and positive look-behinds for the last period, etc

taralx commented 3 years ago

Also did you mean to put these updates on the other issue? 😊

mvasigh commented 3 years ago

More importantly, removing !== undefined is a very big win. How important is it that we allow falsey things to be returned from handle?

This one is tricky... I wanted a short-circuit to block downstream response matching (returning false from a handler/middleware would achieve this), but debatably this is not much different than just throwing with a catch on the router.handle. That said, this would be a breaking change, thus a major version bump... which I'd like to avoid unless we're all convinced this is a worthwhile flag to plant (no falsey responses allowed).

@mvasigh?

I think this discussion is flying a bit over my head right now 😅 need to come back later this week and look at it if there's no merge yet. Or maybe, could one of you all explain like I'm five what the tradeoffs are here?

kwhitley commented 3 years ago

Sorry folks, been a super busy week and change - will see if i can get the ball rolling this weekend!

taralx commented 3 years ago

Hi! Have time to look at this?

kwhitley commented 3 years ago

Sorry dude, had some life emergencies that took me away for a bit - playing catch up now...

So the sticking point I have is getting the (fairly common in my own case) issue of [optional] formats. Still trying to sort out the regex for that. Without solving it I have one of two easy paths:

  1. Leave the bug in place where route /:file.:format? with URL /foo.bar.baz incorrectly returns { file: 'foo.bar.baz', format: undefined }. This only affects filenames with periods in them... when an optional flag on the format is added, and is already currently the expected (albeit incorrect, IMO) behavior in production. I'm thinking we just do this, with a caveat/disclaimer added to the documentation to explain the edge case.
  2. Introduce a breaking change by dropping support for optional file formats... meaning this would no longer be supported /:file.:format?.
kwhitley commented 3 years ago

Since we've been developing (on your concept) in parallel, I'm going to merge this... then merge in my own tweaks on top. Want you to get max line credit! 🥇 :)