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

Proposal: Return Into<Response> from endpoints #596

Closed
wants to merge 2 commits into from

Conversation

jbr
Copy link
Member

@jbr jbr commented Jun 14, 2020

Contingent on #570. With that PR merged, we can convert any Result into a Response, which means that we can allow endpoints to return Into<Response>. This means that hello world examples become app.at("/").get(|_| async { "hello world" }) again, but for endpoints with error handling logic that requires ?, they can return Result<generic Into<Response>, generic Into<tide::Error>>.

The only breaking change people will run into with this is that for closure-style endpoints that previously inferred a return type, they will need to change Ok(into_response) into tide::Result::Ok(into_response) or if ? is unused, just return into_response. This is because rust cannot infer what the error type is with just |_| async { Ok(…) }.

I personally believe that this tradeoff is worthwhile. My hunch is that most route handlers that are fallible also already have explicit Result signatures, and that the ability to skip Ok() in infallbile endpoints is a valuable reduction of boilerplate.

For async fn route handler definitions with an explicit Result<Response> return signature, this doesn't break any code, but will optionally allow any infallible handler to return Response (or anything that's Into<Response>).

To see how this would impact actual usage, check out the updated examples and tide code.

This will also play well with #580, as it will allow

    app.at("/hello").get(|_| async {
        Response::builder(200)
            .header("header", "value")
            .body(json!({ "returning": "a builder" }))
    });

All changes specific to this PR are in this commit: 7a68050

@jbr jbr force-pushed the result-into-response branch 2 times, most recently from 7a68050 to 46c9905 Compare June 14, 2020 21:49
@jbr jbr marked this pull request as draft June 14, 2020 21:55
@jbr jbr force-pushed the result-into-response branch from 46c9905 to 214bd70 Compare June 14, 2020 22:10
@jbr jbr force-pushed the result-into-response branch from 214bd70 to c0b1f46 Compare June 14, 2020 23:31
@Fishrock123
Copy link
Member

Huh. I tried to do this in Fishrock123#2 but it didn't work out.

@jbr jbr requested a review from yoshuawuyts June 15, 2020 19:07
@jbr jbr added blocked An issue that's blocked on upstream changes design Open design question enhancement New feature or request labels Jun 18, 2020
@yoshuawuyts
Copy link
Member

[...] this means that hello world examples become app.at("/").get(|_| async { "hello world" })

Thanks for opening this @jbr. I think we've talked about this before, but I'm still so-so about this approach. What I fear here is that we're heading so deep into generics territory that if a user encounters an error they will no longer be guided into the right direction -- too many things are valid and the compiler no longer knows what to suggest.

The way the lang team intends to fix this is by going further down the line of the Try trait and Ok-wrapping. That way we get the benefit of not needing to wrap things in Ok, but compiler diagnostics are preserved. One way this could end up looking is that basic examples can be written as such (speculative, the lang team has no consensus on any design yet):

app.at("/").get(async try |_| "hello world");

Try and Ok-wrapping seem reasonably high on the priority list for the lang team, and personally I think it's okay for us to wait on them to make progress here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked An issue that's blocked on upstream changes design Open design question enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants