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

impl Endpoint for Service, for service nesting #364

Merged
merged 7 commits into from
Dec 20, 2019

Conversation

tirr-c
Copy link
Collaborator

@tirr-c tirr-c commented Dec 6, 2019

This is a half-baked version of nesting. Need to strip the path prefix, but I feel this is a good first step! Nevermind, I managed to write necessary mechanics for nesting.

  • HttpService impl is refactored to use Endpoint impl internally.
  • Added Route::strip_prefix. When used, this will strip the route path prefix from request before passing it to the endpoint.

With this PR, we can finally nest Tide services!

let mut inner = tide::new();
inner.at("/").get(|_| async { "root" });
inner.at("/foo").get(|_| async { "foo" });
inner.at("/bar").get(|_| async { "bar" });

let mut outer = tide::new();
outer
    .at("/nested")
    .strip_prefix() // catch /nested and /nested/*
    .get(inner.into_http_service()); // the prefix /nested will be stripped here

Might need a better name for strip_prefix, like prefixed?

@tirr-c tirr-c changed the title impl Endpoint for Service impl Endpoint for Service, for service nesting Dec 8, 2019
@eignnx
Copy link

eignnx commented Dec 8, 2019

I wonder if something like nested_service would be clearer than strip_prefix?

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

This patch looks good! I think we can probably merge it as-is.

Looking Ahead

I'm curious if we could move forward from here though. Ideally folks could write something like this, and it would just work:

let mut inner = tide::new();
inner.at("/").get(|_| async { "root" });
inner.at("/foo").get(|_| async { "foo" });
inner.at("/bar").get(|_| async { "bar" });

let mut outer = tide::new();
outer.at("/nested").get(inner);

But we'd need to implement two changes for this:

  • Either implement HttpService for Server, or introduce a new trait: IntoHttpService that knows how to convert something into an http service. It'd be neat if we could remove the Server / Service distinction.
  • Make it so we can discern between Server and regular Endpoints without needing to toggle methods manually. Maybe we could add this as a method on Endpoint that is called once when passed into a handler.

I think if we put those together, nesting services will become something that will become incredibly intuitive!

src/server/route.rs Show resolved Hide resolved
@yoshuawuyts
Copy link
Member

@tirr-c oh, also another question: on Discord someone asked whether we supported middleware on sub-routes, and you mentioned this patch would add support for that. So it looks like this patch specifically doesn't add a middleware method on Route; but it does seem like that would be possible to add as a follow-up?

@tirr-c
Copy link
Collaborator Author

tirr-c commented Dec 14, 2019

@yoshuawuyts I think this is good to merge now! Marked strip_prefix as unstable, as its name might change in the near future.

And yeah, per-route middleware shouldn't be hard to implement. I'll add it as a follow-up.

@yoshuawuyts yoshuawuyts merged commit e0e1dd7 into http-rs:master Dec 20, 2019
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.

3 participants