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

feat: add catch all routes #197

Merged
merged 7 commits into from
Sep 5, 2023
Merged

feat: add catch all routes #197

merged 7 commits into from
Sep 5, 2023

Conversation

mtt-artis
Copy link
Contributor

@mtt-artis mtt-artis commented Aug 20, 2023

hey 👋

this is my first contribution to a rust project.

Description

This PR introduces a catch-all route segment to the route definitions. The catch-all segment is defined as [...segment] and allows for handling routes that haven't been matched by previous patterns.

i don t know how the actix server should behave when two catch-all-routes are defined
ex:

  • /[...segment_a].js
  • /[...segment_b].js
  • /[...segment]/other.js

but it is quite the same with dynamic params

  • /api/[id]/index.js
  • /api/[slot]/index.js

Testing

  • Added unit tests to ensure proper functionality of the catch-all route.
  • Verified that existing tests pass without regressions.

Checklist

  • Code compiles and passes tests locally
  • Unit tests cover the new functionality
  • Documentation is updated to reflect the new feature

It closes #199

@vmwclabot
Copy link

@mtt-artis, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@vmwclabot
Copy link

@mtt-artis, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

@vmwclabot
Copy link

@mtt-artis, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement. Reject reason:

Please provide name (First + Last) and Address (Country/State/Town).

Copy link
Contributor

@Angelmmiguel Angelmmiguel left a comment

Choose a reason for hiding this comment

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

Hello @mtt-artis !

Thank you very much for this amazing PR! This feature is really valuable and it brings more parity with other frameworks like NextJS. I'm going to create a new issue for this and link it to the PR.

Regarding the CLA, you need to complete the missing data. Sorry for any inconvenience on the process!

crates/router/src/route.rs Outdated Show resolved Hide resolved
crates/router/src/route.rs Outdated Show resolved Hide resolved
crates/router/src/route.rs Show resolved Hide resolved
@Angelmmiguel Angelmmiguel self-assigned this Aug 21, 2023
@Angelmmiguel Angelmmiguel added this to the v1.5.0 milestone Aug 21, 2023
@Angelmmiguel Angelmmiguel added the 🚀 enhancement New feature or request label Aug 21, 2023
@mtt-artis
Copy link
Contributor Author

hello @Angelmmiguel
i had some changes so that route /sub/[id] is better than route /[id]/sub
if you don't like it, i'll revert and fix the
(Some(_), Some(_)) => return RouteAffinity::CanManage(i32::MAX - depth),

@vmwclabot
Copy link

@mtt-artis, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

@vmwclabot
Copy link

@mtt-artis, VMware has approved your signed contributor license agreement.

Copy link
Contributor

@Angelmmiguel Angelmmiguel left a comment

Choose a reason for hiding this comment

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

hello @Angelmmiguel
i had some changes so that route /sub/[id] is better than route /[id]/sub
if you don't like it, i'll revert and fix the
(Some(), Some()) => return RouteAffinity::CanManage(i32::MAX - depth),

Hello @mtt-artis!

Thank you for the refactor and all the changes to support "catch all" routes. The current approach simplifies how wws retrieves the right worker on runtime. It doesn't need to compute the affinity of all the routes based on the given path on every request.

I added some minor comments, but everything LGTM! Adding @ereslibre also as a reviewer to this PR before merging it :)

crates/router/src/lib.rs Show resolved Hide resolved
crates/router/src/route.rs Outdated Show resolved Hide resolved
crates/router/src/route.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @mtt-artis! It's looking great. Some comments from my side.

crates/router/src/lib.rs Show resolved Hide resolved
crates/router/src/route.rs Outdated Show resolved Hide resolved
crates/router/src/route/segment.rs Outdated Show resolved Hide resolved
crates/router/src/route/segment.rs Outdated Show resolved Hide resolved
crates/router/src/route/segment.rs Outdated Show resolved Hide resolved
crates/router/src/route/segment.rs Outdated Show resolved Hide resolved
crates/router/src/route/segment.rs Show resolved Hide resolved
crates/router/src/route.rs Show resolved Hide resolved
@mtt-artis
Copy link
Contributor Author

Thank you for the code review and the advices.
I've been rusting for a few weeks and I like it ;P
I'll try to be there tomorrow, if you have any other comments.

@Angelmmiguel Angelmmiguel modified the milestones: v1.5.0, v1.6.0 Sep 1, 2023
Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

LGTM besides the suggested 'satic' -> 'static' spelling changes.

Thank you @mtt-artis!

crates/router/src/route/route_type.rs Outdated Show resolved Hide resolved
crates/router/src/route/route_type.rs Outdated Show resolved Hide resolved
crates/router/src/route/route_type.rs Outdated Show resolved Hide resolved
crates/router/src/route/route_type.rs Outdated Show resolved Hide resolved
crates/router/src/route/route_type.rs Outdated Show resolved Hide resolved
crates/router/src/route.rs Outdated Show resolved Hide resolved
crates/router/src/route.rs Outdated Show resolved Hide resolved
crates/router/src/route.rs Outdated Show resolved Hide resolved
crates/router/src/route.rs Outdated Show resolved Hide resolved
crates/router/src/route.rs Outdated Show resolved Hide resolved
@mtt-artis
Copy link
Contributor Author

Sorry for that... Will fix it tonight

@ereslibre
Copy link
Contributor

There is only one remaining linting issue.

@mtt-artis
Copy link
Contributor Author

Sorry again. You can fix it yourself if you want. Otherwise I'll do it late at night 😁

@ereslibre
Copy link
Contributor

Sorry again. You can fix it yourself if you want. Otherwise I'll do it late at night 😁

Don't worry! Thanks :), I have pushed a fix, it should be fine now, sorry for suggesting deriving PartialOrd, it was my bad since I didn't run the linter myself :)

@ereslibre
Copy link
Contributor

The only thing missing for this feature to be considered complete is to write docs at https://github.com/vmware-labs/wasm-workers-server/tree/main/docs.

I think is fine to address it on a separate PR.

@ereslibre ereslibre merged commit 85309ee into vmware-labs:main Sep 5, 2023
@ereslibre
Copy link
Contributor

@mtt-artis, again thanks for your contribution! And a very nice Rust contribution for being your first one on this language!

I have filed #213 too in case you also want to document this feature. I can take over on that if you prefer, as you wish :)

@mtt-artis
Copy link
Contributor Author

It's cool if you take over.
Thanks for helping me improve by reviewing this pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Catch all" workers
4 participants