pubnub / javascript

PubNub JavaScript SDK docs https://www.pubnub.com/docs/sdks/javascript
Other
553 stars 401 forks source link

PublishParameters message type too strict #412

Closed yo1dog closed 3 weeks ago

yo1dog commented 1 month ago

It looks like the Payload type used by PublishParameters.message attempts to ensure the value contains only valid JSON types, but using this type on inputs prevents users from utilizing the JSON serialization layer. Namely, classes/objects with custom JSON serialization via toJSON(): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#tojson_behavior

For example, instances of Date. Another example is that undefined is not allowed even though JSON serialization excludes those properties automatically.

I suggest using unknown instead of Payload.

The workaround is to use the any trapdoor which defeats the purpose of the typing entirely.

parfeon commented 3 weeks ago

Thank you for reaching out to us.

string is the output of JSON.stringify which is an acceptable data type for PublishParameters.message. According to the RFC 8259 JSON values should be one of:

Serialization / deserialization expected to be done by user.

yo1dog commented 3 weeks ago

The user can take advantage of JSON.stringify()'s behavior of respecting toJSON when serializing values. This works just fine: pubnub.publish({message: {fubar: new Date()}) and results in {"fubar":"2024-10-29T13:22:10.662Z"}. However, the new typings now prevents this.

According to the new typing, in order to support dynamic objects, the user must walk the entire object tree and convert all properties to RFC 8259 values first. But this is exactly what JSON.stringify() already does, so it seems redundant to force the user to implement their own version of this as well.


string is the output of JSON.stringify which is an acceptable data type for PublishParameters.message.

Serialization / deserialization expected to be done by user.

Yes, but currently the PubNub JavaScript SDK forces the user to use its internal method of serialization and deserialization. That is, the user must provide a value which will be passed to JSON.stringfy(). The user cannot fully serialize themselves and pass the resulting JSON string because prepareMessagePayload would double serialize it by passing it through JSON.stringfy() again. So while PubNub expects serialization to be handled by the user, the JavaScript SDK does not allow the user to actually do the serialization. Instead, the user must coerce values into something that the SDK will then serialize on their behalf.

For example, it the user already had a JSON string from another source, the user must parse the JSON string using JSON.parse(), pass the result to the SDK, only for the SDK to immediately serialize it back to a string via JSON.stringify(). Same problem if the user uses a custom JSON serializer.

Besides being redundant, deserializing and reserializing this way can actually change the message payload because RFC 8259 allows implementations to vary. For example: "This specification allows implementations to set limits on the range and precision of numbers accepted." If the source JSON is {"fubar":9200000000000000001} and the user is forced to deserialize the fubar 64bit integer to a JavaScript number, precision is lost and and the resulting reserialized JSON is {"fubar":9200000000000000000} (off by 1).

parfeon commented 3 weeks ago

I still don't understand what is the issue and what redundant calls are done.

User provides a valid JSON object or string (which can be done from dynamic object with JSON.stringify() where they will get all conversion out-of-box). For precision issues, users have a choice:

yo1dog commented 3 weeks ago

User provides a valid JSON object or string ... call JSON.stringify() and use what it will return with publish

No, users cannot provide a JSON string to publish. As the docs say: https://www.pubnub.com/docs/sdks/javascript/api-reference/publish-and-subscribe#message-data

Don't use JSON.stringify It is important to note that you should not use JSON.stringify() when sending signals/messages via PubNub. Why? Because the serialization is done for you automatically. Instead just pass the full object as the message payload. PubNub takes care of everything for you.

Indeed, if a user were to pass a JSON string to publish, prepareMessagePayload would double serialize it by passing it through JSON.stringfy() again.

https://github.com/pubnub/javascript/blob/c47f202c2966408e221267ebfd142c64a6ddde85/src/core/endpoints/publish.ts#L214-L221

pubnub.publish({message: JSON.stringify({fubar: new Date()})) results in "{\"fubar\":\"2024-10-29T13:22:10.662Z\"}"

parfeon commented 3 weeks ago

@yo1dog this issue is addressed in v8.2.10