Skip to content

Define a Serializer to JS values#2996

Closed
Centril wants to merge 2 commits into
centril/v8-deserializerfrom
centril/v8-serializer
Closed

Define a Serializer to JS values#2996
Centril wants to merge 2 commits into
centril/v8-deserializerfrom
centril/v8-serializer

Conversation

@Centril
Copy link
Copy Markdown
Contributor

@Centril Centril commented Jul 28, 2025

Description of Changes

Defines Serializer, which can serialize to a v8::Value from a SATS value.

API and ABI breaking changes

None

Expected complexity level and risk

Doesn't interact with existing code.

Testing

Roundtrip-tests are added, both for some unit cases and for any random AT/AV pair (proptest).

@Centril Centril force-pushed the centril/v8-serializer branch from 1d3469b to 292bc4a Compare July 28, 2025 19:45
@Centril Centril changed the base branch from master to centril/v8-deserializer July 28, 2025 19:46
Comment on lines +133 to +156
fn serialize_variant<T: Serialize + ?Sized>(
mut self,
tag: u8,
var_name: Option<&str>,
value: &T,
) -> Result<Self::Ok, Self::Error> {
// Serialize the payload.
let value_value: Local<'s, Value> = value.serialize(self.reborrow())?;
// Figure out the tag.
let tag_value: Local<'s, Value> = intern_field_name(self.scope, var_name, tag as usize).into();
let values = [tag_value, value_value];

// The property keys are always `"tag"` an `"value"`.
let names = [
self.key_cache.tag(self.scope).into(),
self.key_cache.value(self.scope).into(),
];

// Stitch together the object.
let prototype = v8::null(self.scope).into();
let object = Object::with_prototype_and_properties(self.scope, prototype, &names, &values);
seal_object(self.scope, &object)?;
Ok(object.into())
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gut says this should have special handling for options, the same way that the deserialize path does.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that, but a problem here is that we actually do not have enough information to tell that the type is option here because you don't know about the other variant. For example, for .serialize_variant(0, Some("none"), payload), it could be option, but you could also have 3 variants instead of 2. Threading this information through SATS is currently not trivial. We might want to consider e.g., adding AlgebraicTypeLayout::Optional(_), which could actually simplify things down the road. But for now, I'd rather remove the special case from the deserialization end instead.

(See also #2985 (comment).)

@bfops bfops added typescript-modules release-any To be landed in any release window labels Aug 4, 2025
@Centril Centril closed this Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-any To be landed in any release window typescript-modules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants