Porting to Serde 1.0 #38
Conversation
Before extending `estree` to export estree ASTs, introducing Serde 1.0, which should make our life simpler, by letting us use macro `json!`.
|
Ah, well, and a second commit that introduces a first attempt to serialize to ESTree. |
|
Ping @dherman, @RReverser ? |
| _ => { return type_error("number or null", self.ty()); } | ||
| }) | ||
| match *self { | ||
| Value::Number(ref v) if v.is_f64() => Ok(Some(v.as_f64().unwrap())), |
There was a problem hiding this comment.
Can't we use as_f64 for everything without conditions? (that's anyway the only number type we can support)
There was a problem hiding this comment.
Well, in the current version of serde_json, as_f64() returns None if the number is PosInt or NegInt.
As a side-note, I don't understand why they have PosInt or NegInt in the first place, since these don't exist in JS, nor why as_f64() doesn't attempt to convert.
| /// A container used to represent some piece of data being | ||
| /// serialized. Future versions may add options to the | ||
| /// serialization format. | ||
| pub struct Serialization<'a, T> where T: 'a { |
There was a problem hiding this comment.
This is the file where I got stuck previous time. Can you please explain what are these containers doing / why are they necessary? Can't we just return own json!() generated structure and let consumer call .serialize on the target serializer they wish to serialize to?
There was a problem hiding this comment.
There are two problems. First, I don't think that you can simply implement serde::ser::Serialize for easter in estree. Unless the rules have changed, you can only implement an external trait for a type you have defined locally.
The second reason is more of an ulterior motive. There are at least two widely-used versions of ESTree available: Esprima's and Babel's. Ideally, it would be nice to be able to export to either. For this purpose, we need a way to pass options during serialization. Since Serde doesn't support passing options out of the box, the idea would be to pass them as a field of Serialization.
There was a problem hiding this comment.
No, I didn't mean for easter, but just return a wrapper that consumer can further call .serialize on.
There are at least two widely-used versions of ESTree available: Esprima's and Babel's.
Babel has own AST format which they forked from ESTree, but it's not ESTree anymore which is clearly defined in the spec.
However, I don't see it as a big problem to just return result of json! macro (essentially a Map) and then consumer can just provide own Serializer instead that would transform nodes before writing them out as strings, or accept serialization options or anything else.
There was a problem hiding this comment.
TL;DR - I'm suggesting to just provide a trait that would allow to convert any Easter node to serde_json::Map (via a macro), and then we don't need to care about serialization config in the Esprit itself, but instead let consumer decide what to do with that JSON tree further.
There was a problem hiding this comment.
As a side-note, while Babel has forked from ESTree, I realized that I personally need a Babel output, rather than a ESTree output.
| impl<'a> Serialize for Serialization<'a, Op<BinopTag>> { | ||
| fn serialize<S: Serializer>(&self, serializer: S) -> ::std::result::Result<S::Ok, S::Error> { | ||
| use easter::punc::BinopTag::*; | ||
| match self.data().tag { |
There was a problem hiding this comment.
Maybe best to put string->operator conversions as part of operators themselves so that they could be potentially reused for other serialization targets?
|
@Yoric Thanks for your PR! Sorry, got stuck previous time during review and then forgot about it. Added some comments. |
2 participants
Before extending
estreeto export estree ASTs, introducing Serde 1.0, which should make our life simpler, by letting us use macrojson!.