adds initial support for json literals #523
Conversation
| fn from(json_value: serde_json::Value) -> Self { | ||
| Response::new(StatusCode::Ok) | ||
| .body_json(&json_value) | ||
| .unwrap_or_else(|_| Response::new(StatusCode::InternalServerError)) |
There was a problem hiding this comment.
I'm not actually sure if this is reachable, since a serde_json::Value should basically be the safest thing to turn into json, as the intermediate representation before stringification
There was a problem hiding this comment.
we should probably use the Status trait here instead so we don't lose the original error message.
There was a problem hiding this comment.
As far as returning status: From<serde_json::Value> for Response needs to return a Response, not a tide::Error. I can imagine the value of adding an Option<tide::Error> on Response (or using ext in a systematic way) and adding a TryInto or downcast, but that's way beyond the scope of this PR. If people want to retain the original error message from transforming a json literal into a Response, they'll need to explicitly do the body_json call for now
| @@ -1,2 +1,3 @@ | |||
| //! The Tide prelude. | |||
| pub use http_types::Status; | |||
| pub use serde_json::json; | |||
There was a problem hiding this comment.
would be nice to make serde_json a feature dependency, even if enabled by default
There was a problem hiding this comment.
I expected to need to do that when starting this pr, but discovered it was already a normal dependency. We'd probably want to gate json_body on this as well
There was a problem hiding this comment.
Yep, that would be a good improvement, given that serde_json is not the lightest dependency
There was a problem hiding this comment.
Added a "json" feature, enabled by default, and gated everything json-related in tide on it. Probably could stand to do the same for http-types as well
jbr
changed the title
|
@jbr it seems tests are still failing; I'd love to get these to pass so we can merge this PR. |
|
@yoshuawuyts Fixed! |
|
Wait actually: I was just digging in on adding a similar json feature for http-types, but ran into this issue: This header takes a json value, which means http-types csp stuff needs serde_json, which means tide always is going to have a serde_json dependency, in which case is there any reason to add the feature flag? |
|
Heh, yeah probably always supporting JSON is fine. Serde even works on embedded systems, so don't think there's really any practical reason to make this opt-out. |
|
I'll back out the feature-gate parts of this pr |
jbr
changed the title
No description provided.