Skip to content
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

PublishParameters message type too strict #412

Closed
yo1dog opened this issue Oct 14, 2024 · 5 comments · Fixed by #417
Closed

PublishParameters message type too strict #412

yo1dog opened this issue Oct 14, 2024 · 5 comments · Fixed by #417
Assignees
Labels
priority: low This PR should be reviewed after all high and medium PRs. status: in progress This issue is being worked on. type: refactor This PR contains refactored existing features.

Comments

@yo1dog
Copy link

yo1dog commented Oct 14, 2024

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
Copy link
Contributor

parfeon commented Oct 29, 2024

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:

  • object
  • array
  • number
  • string
  • false / true / null

Serialization / deserialization expected to be done by user.

@parfeon parfeon added status: rejected This issue is considered rejected. It will not be worked on. priority: low This PR should be reviewed after all high and medium PRs. labels Oct 29, 2024
@parfeon parfeon self-assigned this Oct 29, 2024
@parfeon parfeon added the type: refactor This PR contains refactored existing features. label Oct 29, 2024
@yo1dog
Copy link
Author

yo1dog commented Oct 29, 2024

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
Copy link
Contributor

parfeon commented Oct 30, 2024

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:

  • call JSON.stringify() and use what it will return with publish
  • make bigint a string which won't cause precision fluctuation.

@yo1dog
Copy link
Author

yo1dog commented Oct 30, 2024

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.

private prepareMessagePayload(payload: Payload): string {
const { crypto } = this.parameters;
if (!crypto) return JSON.stringify(payload) || '';
const encrypted = crypto.encrypt(JSON.stringify(payload));
return JSON.stringify(typeof encrypted === 'string' ? encrypted : encode(encrypted));
}

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

parfeon added a commit that referenced this issue Oct 30, 2024
Fix definition of type which represent message actions received from history and list of users which added action of specific type and value to the message.

Closes #407

refactor(types): remove indexed signature for publish

Remove redundant indexed signature from publish message parameters type definition.

Closes #413

refactor(types): add serializable objects to `Payload` type

Extend `Payload` type definition with objects which can be serialized by `JSON.stringify` using `toJSON()` methods.

Closes #412

refactor(types): aggregate generated types definitions

Aggregate multiple types definitions into single type definition type with proper type names and namespaces.

Closes #405 #409 #410

refactor(types): add missing Subscribe Event Engine types

Add Subscribe Event Engine and Event Listener types to the bundled types definition file.

Closes #377
@parfeon parfeon added status: in progress This issue is being worked on. and removed status: rejected This issue is considered rejected. It will not be worked on. labels Oct 30, 2024
@parfeon
Copy link
Contributor

parfeon commented Oct 31, 2024

@yo1dog this issue is addressed in v8.2.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low This PR should be reviewed after all high and medium PRs. status: in progress This issue is being worked on. type: refactor This PR contains refactored existing features.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants