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

Feature: Implementing Render for Result #52

Open
d4h0 opened this issue Feb 2, 2021 · 2 comments
Open

Feature: Implementing Render for Result #52

d4h0 opened this issue Feb 2, 2021 · 2 comments
Labels
Type: Breaking Change Includes breaking changes

Comments

@d4h0
Copy link

d4h0 commented Feb 2, 2021

Hi,

Another small improvement would be, implementing Render for Result.

Basically, if a result is Ok(_), the value is extracted and rendered. And if there is an error, the template would fail with that error, similar to the ? operator in Rust.

Would that be possible?

@Kogia-sima Kogia-sima added the Type: Breaking Change Includes breaking changes label Feb 3, 2021
@Kogia-sima
Copy link
Member

One of the problem is, should we support any Error types, or RenderError only?

In my opinion, template engines should avoid throwing unexpected error as much as possible, because they are mostly used in web applications, and developers must handle all errors raised during response. Good solution is implementing Render for Result::<T: Render, RenderError> only, not for Result::<T: Render, E: Error>.

What do you think about this? Is there any use case for implementing Render for errors which does not implement Render trait?

@d4h0
Copy link
Author

d4h0 commented Feb 3, 2021

One of the problem is, should we support any Error types, or RenderError only?

Ideally, custom errors would be supported. That would give error handling code (within or outside sailfish) more information to handle the error properly.

In my opinion, template engines should avoid throwing unexpected error as much as possible, because they are mostly used in web applications, and developers must handle all errors raised during response.

Because sailfish supports regular Rust code, many errors can happen. If only RenderError is supported, all the error handling would need to happen within templates, which could be pretty un-ergonomically.

It's very common to use an error type like AnyHow in application code, which passes errors up to be handled by main. I don't think web applications are any different in that. There always can be errors, and in that cases you would return an "Internal Server Error" with a status code of 500. This would be implemented at the very top, where the root template is rendered. And there you also would have some logging functionality, to collect what errors are happening.

I see two advantages in supporting custom errors:

(1) You can pass them back all the way to the original caller (like AnyHow). This would be most useful for unrecoverable errors.

(2) To handle errors in parent template. This would be very useful in combination with my proposal for including templates.

This means, for example, the calling template could:

  1. Create an "inline HTML" error (an error message that's embedded into the HTML)
  2. Show alternative content
  3. Create a specialized error, and return this error to the parent template
  4. Return the error unchanged (if it's unrecoverable)
  5. Do some other action

For 1), for example, this could be including the error itself (the Rust error) into the HTML (which would be very useful while developing an app). Or showing an error message to the end-user ("Couldn't load widget X").

For 3), this could be a 404 error if, for example, an essential resource (that's selected using user input) isn't found. Or this could be a redirect to a different resource.

At the moment, the only errors that can be returned are error strings, or fmt::Error, which makes it difficult to handle errors properly, or efficiently. One workaround would be, to return errors as JSON strings, but that is sub-optimal, I'm sure you would agree.

I'm not sure, what the best way to implement my proposal would be.

It seems, it would be possible to add a type parameter C and a new variant Custom<C> to sailfish::RenderError. Then users could define an error enum, or something like AnyHow as C, and return RenderError::Custom(foo), or maybe even implement Into<RenderError> for their error type.

This would add the ability to sailfish, to handle errors similarly to how errors are handled in regular Rust (which is a big advantages Rust has, in my opinion, in comparisons to most other programming languages).

It would probably be a breaking change, but I believe this would be very worth it.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking Change Includes breaking changes
Projects
None yet
Development

No branches or pull requests

2 participants