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

Merge PR for #34, add or_else_any_method explicit handlers #46

Merged
merged 10 commits into from Sep 15, 2021

Conversation

nilslice
Copy link
Contributor

This PR builds on #34, adding explicit route handler for non-match patterns using the not_found and not_found_async methods.

It also modifies the worker-sandbox Worker to narrow its match handlers to more specific HTTP methods to allow for better testing of wildcard patterns and not_found handler.

@nilslice
Copy link
Contributor Author

@fkettelhoit - if possible, it would be great to get your review on this to make sure it captures all of what you were aiming to have supported with your PR.

worker/src/router.rs Outdated Show resolved Hide resolved
worker/src/router.rs Outdated Show resolved Hide resolved
@fkettelhoit
Copy link
Contributor

@fkettelhoit - if possible, it would be great to get your review on this to make sure it captures all of what you were aiming to have supported with your PR.

Looks good! I was thinking a bit about how the not_found case might be generalized, the result is PR #47. But I am honestly not sure whether that would add significant value over the more specific version implemented here, in my opinion either would be fine.

(I think there is a larger issue at play here: How much should be handled directly in a single router vs. composing multiple routers? Thanks to the functional nature of the router, it is easy to map the result of one router and use another router for any 404 response, for example, which can already solve most catchall cases. But there is still value in providing an API that solves most common situations in a single router, in my opinion, because compositions of different routers are not always the most obvious solution. I would say that all in all the current PR strikes a good balance there.)

Generalize `not_found` handler to handle arbitrary patterns
@nilslice nilslice changed the title Merge PR for #34, add not_found explicit handlers Merge PR for #34, add or_else_any_method explicit handlers Sep 15, 2021
@nilslice nilslice merged commit 00ce8ce into main Sep 15, 2021
@nilslice nilslice deleted the merge/pr-34 branch September 15, 2021 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants