[WIP] feat: dispatch_json in core
#4434
Conversation
afinch7
force-pushed
the
dispatch_json_core
branch
2 times, most recently
from
1998d2c
to
6612fcb
Compare
There was a problem hiding this comment.
@afinch7 could you scale back this PR to just add a separate crate for dispatch_json without changing CLI/plugins? It can be another example in deno_core that uses dispatch json. It will be much easier to iterate on and land
| pub type JsonResult<E> = Result<Value, E>; | ||
|
|
||
| pub type AsyncJsonOp<E> = Pin<Box<dyn Future<Output = JsonResult<E>>>>; | ||
|
|
||
| pub enum JsonOp<E: JsonError> { | ||
| Sync(Value), | ||
| Async(AsyncJsonOp<E>), | ||
| /// AsyncUnref is the variation of Async, which doesn't block the program | ||
| /// exiting. | ||
| AsyncUnref(AsyncJsonOp<E>), | ||
| } |
There was a problem hiding this comment.
Is the type parameter required here? It leads to Result<JsonOp<OpError>, OpError> declarations... How about:
pub type JsonResult = Result<Value, Into<JsonSerializableError>>;
struct JsonSerializableError {
kind/id: i32,
message: String
}It would require users to implement only single trait for custom error type
There was a problem hiding this comment.
This doesn't really simplify anything afaik.
- Can't implement
Intodirectly for foreign error kinds I.E.impl Into<JsonSerializableError> for url::ParseError Intois a trait therefore it isn't sized(would need to be boxed)Fromis required for use of the?operator
core/dispatch_json.rs
Outdated
| /// Internal representation of an error from inside or outside of json dispatch. | ||
| /// All errors get converted to this format before being serialized. | ||
| #[derive(Debug, Clone, Serialize)] | ||
| struct FinalJsonError { | ||
| pub kind: i64, | ||
| pub message: String, | ||
| } | ||
|
|
||
| impl<E: JsonError + Send + Sync> From<E> for FinalJsonError { | ||
| fn from(e: E) -> FinalJsonError { | ||
| let kind = match e.kind() { | ||
| JsonErrorKind::Kind(k) => { | ||
| assert!(k != 0, "kind = 0 is reserved for UnKind"); | ||
| k as i64 | ||
| } | ||
| JsonErrorKind::UnKind => 0, | ||
| }; | ||
| FinalJsonError { | ||
| kind, | ||
| message: e.msg(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is really complex, PTAL at my comment below, I believe we could get rid of it completely
There was a problem hiding this comment.
This is internal only. Not relevant to implementations of dispatch_json. The existing implementation in cli only requires a single trait implementation for OpError:
impl JsonError for OpError {
fn kind(&self) -> JsonErrorKind {
(self.kind as u32).into()
}
fn msg(&self) -> String {
self.msg.clone()
}
}
afinch7
force-pushed
the
dispatch_json_core
branch
3 times, most recently
from
f0ea28a
to
67bdd57
Compare
afinch7
force-pushed
the
dispatch_json_core
branch
2 times, most recently
from
531b081
to
83b0e00
Compare
|
I still want to land this one. I'm working on some simplifications now. |
afinch7
force-pushed
the
dispatch_json_core
branch
from
83b0e00
to
1f1a882
Compare
afinch7
force-pushed
the
dispatch_json_core
branch
3 times, most recently
from
5ed05bc
to
90e020a
Compare
| pub trait DispatchOpFn: | ||
| Fn(&mut dyn Interface, &[u8], Option<ZeroCopyBuf>) -> Op | ||
| { | ||
| fn box_op(self) -> Box<dyn DispatchOpFn>; | ||
| } | ||
|
|
||
| impl<D: Fn(&mut dyn Interface, &[u8], Option<ZeroCopyBuf>) -> Op + 'static> | ||
| DispatchOpFn for D | ||
| { | ||
| fn box_op(self) -> Box<dyn DispatchOpFn> { | ||
| Box::new(self) | ||
| } | ||
| } |
There was a problem hiding this comment.
It don't know if this really simplifies things enough to be worth it. It's not much harder to just use Box::new. Users will still have to import the trait to use it anyways.
afinch7
force-pushed
the
dispatch_json_core
branch
from
90e020a
to
1c782c3
Compare
afinch7
force-pushed
the
dispatch_json_core
branch
from
1c782c3
to
486cac7
Compare
bartlomieju
added
cli
|
This seems to have created a segfault on windows at exit. It shouldn't really be possible to drop the library as there are still references to it, but for some reason dropping plugin ops at exit causes a segfault only on windows. Edit: It might make sense to land the changes to the plugin api in their own PR. |
|
Closed in favor of #6398 |
heavily based on #3692
This should be mostly working other than plugin support(maybe save that for a follow up).
cc @bartlomieju