-
Notifications
You must be signed in to change notification settings - Fork 81
Add JSON-Safe serialization modes #1308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add JSON-Safe serialization modes #1308
Conversation
Closes RobotWebTools#1307 - Add SerializationMode type ('typed' | 'plain' | 'json') - Implement message_serialization.js with conversion utilities - Add serializationMode option to createSubscription - Export toJSONSafe and toJSONString utility functions
|
@mahmoudalghalayini thanks for submitting the PR with ut and examples, that's great! The overall looks good to me, I will be oof for the rest of the week and will take a closer look early next week. Meanwhile, please feel free to take what we disscussed in #1306, thanks! |
lib/subscription.js
Outdated
| this._callback(msg.toPlainObject(this.typedArrayEnabled)); | ||
| let message = msg.toPlainObject(this.typedArrayEnabled); | ||
|
|
||
| if (this._serializationMode !== 'typed') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some concern about the naming typed here, considering when:
this.typedArrayEnabledisfalsethis._serializationModeistyped
The message may contain plain array even if the array can be typed-array, which may cause confusing.
When typedArrayEnabled is true, rclnodejs won't allocate additional memory from C++ side, instead, the typed array is backed by a Buffer if possible, whose memory was allocated by rcl, but I don't have a proper name in my mind now.
| @@ -0,0 +1,41 @@ | |||
| // Copyright (c) 2024 rclnodejs contributors. All rights reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer unifying the wording here, you can either:
- Add your name, like https://github.com/RobotWebTools/rclnodejs/blob/develop/lib/parameter_service.js
- Use a general name, like https://github.com/RobotWebTools/rclnodejs/blob/develop/lib/utils.js
It's up to you. nit: it's year of 2025 already :)
minggangw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for submitting the PR!
| * @param {boolean} options.enableTypedArray - The topic will use TypedArray if necessary, default: true. | ||
| * @param {QoS} options.qos - ROS Middleware "quality of service" settings for the subscription, default: QoS.profileDefault. | ||
| * @param {boolean} options.isRaw - The topic is serialized when true, default: false. | ||
| * @param {string} [options.serializationMode='default'] - Controls message serialization format: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default makes sence to combine with enableTypedArray
## **Public API Changes**
**New subscription option:**
- `serializationMode: 'default' | 'plain' | 'json'` for `createSubscription()`
**New utility functions:**
- `rclnodejs.toJSONSafe(obj)` - makes objects JSON-friendly
- `rclnodejs.toJSONString(obj)` - converts directly to JSON string
**Fully backward compatible** - existing code works unchanged.
## **Description**
This adds **message serialization modes** to fix a common headache: ROS sensor messages use `TypedArray`s which don't play nice with JSON.
**The Problem:**
If you try to `JSON.stringify()` a LaserScan message, you get `{}` instead of the actual data. This breaks web APIs, logging, and any workflow that needs JSON.
**The Solution:**
Three modes to handle messages differently:
- **`'default'`** Uses native rclnodejs behavior
- **`'plain'`**: Convert TypedArrays to regular arrays - JSON works!
- **`'json'`**: Handle everything (TypedArrays, BigInt, NaN) - fully JSON-safe
[#1307](#1307)
|
@mahmoud-ghalayini this change has landed onto latest v1.6.0, thanks for your contribution! |
Public API Changes
New subscription option:
serializationMode: 'default' | 'plain' | 'json'forcreateSubscription()New utility functions:
rclnodejs.toJSONSafe(obj)- makes objects JSON-friendlyrclnodejs.toJSONString(obj)- converts directly to JSON stringFully backward compatible - existing code works unchanged.
Description
This adds message serialization modes to fix a common headache: ROS sensor messages use
TypedArrays which don't play nice with JSON.The Problem:
If you try to
JSON.stringify()a LaserScan message, you get{}instead of the actual data. This breaks web APIs, logging, and any workflow that needs JSON.The Solution:
Three modes to handle messages differently:
'default'Uses native rclnodejs behavior'plain': Convert TypedArrays to regular arrays - JSON works!'json': Handle everything (TypedArrays, BigInt, NaN) - fully JSON-safe#1307