neon-bindings / neon

Rust bindings for writing safe and fast native Node.js modules.
https://www.neon-bindings.com/
Apache License 2.0
8k stars 283 forks source link

feat(neon): Serde implementation optimized with JSON #953

Closed kjvalencik closed 1 year ago

kjvalencik commented 1 year ago

Based on https://github.com/neon-bindings/neon/pull/701, but with the following changes:

Questions

*For future consideration Now that GAT exist, what might it look like to be able to export function that automatically deserialize arguments and serialize return values?

cx.export_function("greet", |mut cx, (name,): (String,)| Ok(format!("Hello, {name}!")));
antonok-edm commented 1 year ago

@dherman @kjvalencik is there anything blocking here that I can help with? I've been suddenly running into a really weird issue that seems related to neon_serde; I could spend my time and effort debugging and working around that issue but I'd prefer to invest it towards something more useful here if possible.

kjvalencik commented 1 year ago

@antonok-edm that does look like a really strange bug! I may look into it if I get a chance.

As for this PR, I'm going to close it. I've been deliberating over this for a long time and decided that quietly switching to JSON in some circumstances is not the right approach. As demonstrated in your PR, it's already pretty easy for users to use JSON if that's their preference.

I'm going to open a new PR that is direct transcoding (closer to neon-serde) and document the caveats that it may be slower than JSON. Then if optimizations are made available to Node-API (currently, object related ones are private V8 APIs that only built-in JSON can leverage), we can make it faster without changing behavior.

There may still be some benefits of building JSON in, since it's such a common pattern. Do you have any thoughts here? Perhaps something like:

let (Something, Other, String) = cx.args_from_json()?;
antonok-edm commented 1 year ago

There may still be some benefits of building JSON in, since it's such a common pattern. Do you have any thoughts here? Perhaps something like:

let (Something, Other, String) = cx.args_from_json()?;

It doesn't seem like that'd support overloaded signatures (which could be fine, I guess). I think having an equivalent of what I wrote in the json_ffi mod here would be great though.

kjvalencik commented 1 year ago

We discussed this in our last meeting and the plan is to use a newtype approach similar to axum extractors. Something like:

let (a_string, a_number, Json(other_json)): (String, f64, Json<MyStruct>) = cx.args()?;

You could also have extractors that cover overloaded signatures cases like Either<String, f64>.

This covers the case where the value is already serialized JSON, but I see in your example it's calling JSON.stringify first. We could have something that did that, although I'm not sure what to call it (ViaJson?).

My plan is to first implement this and then bring back serde as an extractor type (and without the hacky fallback to JSON).