Skip to content

Define a Deserializer from JS values#2985

Closed
Centril wants to merge 3 commits into
masterfrom
centril/v8-deserializer
Closed

Define a Deserializer from JS values#2985
Centril wants to merge 3 commits into
masterfrom
centril/v8-deserializer

Conversation

@Centril
Copy link
Copy Markdown
Contributor

@Centril Centril commented Jul 24, 2025

Description of Changes

Defines Deserializer, which can deserialize a v8::Value to SATS values.

API and ABI breaking changes

None

Expected complexity level and risk

Doesn't interact with existing code.

Testing

Roundtrip tests will be added in the next PR which will define Serializer.

@Centril Centril force-pushed the centril/v8-deserializer branch from eeca254 to aa8249c Compare July 24, 2025 21:57
Comment thread crates/bindings-macro/src/sats.rs
Comment thread crates/sats/src/de/impls.rs
Comment thread crates/core/src/host/v8/de.rs
Comment thread crates/core/src/host/v8/de.rs Outdated
Comment thread crates/core/src/host/v8/de.rs
@bfops bfops added release-any To be landed in any release window typescript-modules labels Jul 28, 2025
@Centril Centril requested a review from gefjon July 28, 2025 19:43
Comment thread crates/core/src/host/v8/de.rs
Comment thread crates/core/src/host/v8/de.rs
Comment on lines +177 to +180
// We expect a canonical representation of a sum value in JS to be
// `{ tag: "foo", value: a_value_for_foo }`
// with special convenience for optionals
// where we also accept `null`, `undefined` and an object without `tag`.
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.

It feels mildly footgun-ey that putting a tag within an object breaks using the convenience representation, and therefore that Option<enum { Foo, Bar }> must be represented as { "tag": "Some", "value": { "tag": "Foo" } }. I think we probably should require that optionals be null | undefined | bare_value, never the "regular sum" representation.

Copy link
Copy Markdown
Contributor Author

@Centril Centril Jul 30, 2025

Choose a reason for hiding this comment

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

I think we probably should require that optionals be null | undefined | bare_value, never the "regular sum" representation.

If we want to remove the special case, I'd rather opt for always using the "regular sum" repr, as otherwise, you cannot represent Some(None), which is possible in both Rust and C# modules. This will make things slightly less convenient for the module author, but we can add some convenience functions on the TS bindings side.

(See also #2996 (comment).)

Comment thread crates/core/src/host/v8/de.rs Outdated
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.

4 participants