Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: use itty-router for routing requests #125

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flakey5
Copy link
Member

@flakey5 flakey5 commented Jun 21, 2024

Closes #122
Closes #106
Closes #111

TODO

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

@flakey5
Copy link
Member Author

flakey5 commented Jun 21, 2024

cc @ovflowd

@flakey5 flakey5 force-pushed the flakey5/20240606/routing branch 4 times, most recently from 468abd1 to 6073527 Compare June 30, 2024 21:46
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review! Going to review more later :)

src/middleware/originMiddleware.ts Outdated Show resolved Hide resolved
src/middleware/r2Middleware.ts Outdated Show resolved Hide resolved
src/middleware/r2Middleware.ts Outdated Show resolved Hide resolved
src/middleware/r2Middleware.ts Outdated Show resolved Hide resolved
});
}

function getR2Path({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@flakey5 flakey5 force-pushed the flakey5/20240606/routing branch 2 times, most recently from 9536911 to 7feed9d Compare July 4, 2024 18:30
@flakey5 flakey5 marked this pull request as ready for review July 4, 2024 18:30
@flakey5 flakey5 requested review from a team as code owners July 4, 2024 18:30
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should build still be on the codeowners for this file?

@flakey5 flakey5 force-pushed the flakey5/20240606/routing branch 2 times, most recently from faea803 to 16c0b7b Compare July 4, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants