fix: replace unwraps with errors in fullstack WebSockets #5248
Conversation
|
|
||
| /// Error during serialization/deserialization. | ||
| #[error("serde_json error")] | ||
| Json(#[from] serde_json::Error), | ||
|
|
||
| /// Error during serialization/deserialization. | ||
| #[error("ciborium error")] | ||
| Cbor(#[from] ciborium::de::Error<std::io::Error>), |
There was a problem hiding this comment.
These are used nowhere.
packages/fullstack-core/src/error.rs
Outdated
| .status() | ||
| .as_ref() | ||
| .map(StatusCode::as_u16) | ||
| .unwrap_or(DEFAULT_STATUS_CODE), |
There was a problem hiding this comment.
I am not sure how you guys handle these kinds of cases. This code returns a zero status code, which is not very valid (even the StatusCode type accepts only NonZeroU16).
I guess after checking that value.is_status() (which this code does), we could just do something like .expect("status code not present when is_status is true"), but that could theoretically panic, which is exactly what this PR is trying to avoid.
| let response = native::send_request(request, &self.protocols) | ||
| .await | ||
| .unwrap(); | ||
| let response = native::send_request(request, &self.protocols).await?; | ||
|
|
||
| let (inner, protocol) = response | ||
| .into_stream_and_protocol(self.protocols, None) | ||
| .await | ||
| .unwrap(); | ||
| .await?; |
There was a problem hiding this comment.
This is the whole point of this PR. Everything else are required error type conversions.
|
I would prefer we not pull in large deps in the fullstack-core crate. Can we just place the conversions inline in the higher level crates? Placing them too low bloats the compile graph and slows down our compile times. |
|
Sure, is it okay like this? |
2 participants
Similarly to #4916, closes #5247.