src: use itty-router for routing requests

[flakey5]

Closes #122
Closes #106
Closes #111

Fixes dates past April/May on directory listings

TODO

• r2 middleware
• origin middleware
• caching middleware
• tests
• commenting
• custom request type (contains parsed url so we only do it once)

[flakey5]

cc @ovflowd

[ovflowd]

Partial review! Going to review more later :)

[ovflowd]

Shouldnt these be handled by itty router why path regular expressions? Ie:

/some/path/{something}/some/other/matcher

[flakey5]

We are handling parts of the path with itty router here, but that just pulls out some info from the path so that we can use it later.

We still need to take the relevant parts and make the path in the r2 bucket, which is what this function does

[ovflowd]

Can't we use itty router again for that? Like a layered strategy?

So an in-router router? Because that is the part I really wish to become "easy to handle" by just using itty-router

[flakey5]

Maybe? I'm not really sure what that would look like

I think with this approach we're still better off though. It's still a tiny bit messy but a lot more understandable than what we have now and would probably be simpler than having an in-router router imo

I also debated doing something like

router.get('/:rootPath/?:path+', [cached(r2Middleware), originMiddleware]);

that way we can just use a switch statement in getR2Path, which might help with cleaning it up. Performance wise it should be about the same. Might cause some annoyances if we want to add other GET paths to the worker later down the road though

[ovflowd]

I'm still a -1 with this approach, we doing too much logic, and I believe using an inner router (even if it's not really routing but just path matching feels better.

So we remove the wrangle of if/elses to simply having callback statements;

Hence this function itself instead of returning a value, has callbacks... At least for me it becomes easier to read than all these conditions 😅

And also easier to maintain as path matchers/globs are easier to maintain imo

[ovflowd]

Let me know, @flakey5 what you decide to do here :) -- Since I would really appreciate some abstraction here.

[flakey5]

How about we make an issue and take a look at this later on, just to limit this pr from growing any larger 😅 ?

I think I know an approach that would work for this though

[ovflowd]

Sorry for the delayed review. Gonna do it tomorrow! I was swamped with work

[flakey5]

Sorry for the delayed review. Gonna do it tomorrow! I was swamped with work

No worries, thank you!

[ovflowd]

Isn't this auto-generated? If yes, shouldn't it be on the gitignore?

[flakey5]

It is but with the way it's generated it needs to be committed. The update-latest-versions.js utilizes the s3 api, which requires an access key & secret for the dist-prod bucket.

[ovflowd]

Can't this be generated during the CI that deploys the Worker?

[flakey5]

We could if we replaced the file with a placeholder, but imo committing a placeholder vs what's going to be deployed, I'd prefer the later since it'd be easier to debug if any issues pop up.

Or unless you're suggesting we move the update redirect links action into the deployment one (i.e. as a step we update & commit the file)? I'd be in favor of this tbh

Regardless we'd need something for the file in git or else we couldn't import it in the code, I also think this might be out of the scope of this pr since this is already the way it's been done and this pr is already fairly big

[ovflowd]

I feel that the CI could be responsible for generating the file and ensuring when deployed it is correct.

Instead of committing this within the repo; This sounds like the sort of file that shouldn't be committed.

[flakey5]

This sounds like the sort of file that shouldn't be committed.

How would local testing work then, do we just want a placeholder file? It would throw if we omitted it entirely, https://github.com/nodejs/release-cloudflare-worker/pull/125/files#diff-d62c9ceba913680e28434574c470caed6e5539f9afd659b538f45840e1ad20caR7

[ovflowd]

LGTM! Apologies for taking so long to finish reviewing this. Please address my comments first before proceeding to merge this. Here are some caveats:

• Please add more inline docs whenever possible, some logic might not be understood by newcomers and some insider' context of why a said piece of code was done in a certain way is done this way might be not easy to follow.
• Update empty lines to be consistent
• Ensure you are properly using import type
• Avoid using + operator with strings
• Separate logic from return statements, assign dedicated constants for the values of the keys of return statements that have complex objects
• Ensure everything is properly typed
• Make constants for things that should be constants
• Ensure you are covering edge scenarios of your logic, cover things with test suites, in doubt ask Copilot.