search

socketio / socket.io

Realtime application framework (Node.JS server)
https://socket.io
MIT License
61.3k stars 10.13k forks source link

Partial omission of emitted object if jsonification fails #5225

Open eldir opened 3 weeks ago

eldir commented 3 weeks ago

Describe the bug When passing an object to an emit that contains a function reference it just quietly gets omitted.

To Reproduce

function test_function() {
  return null;
}

const data = {
  key_one: "value 1",
  key_two: test_function
}

socket.emit("test", data);

Checking traffic will show that test gets a message containing only

{
  "key_one": "value 1"
}

With no error or even warning on the console.

Expected behavior If jsonification fails (which is what I suspect is happening here) I would expect at least a warning or a full transmission failure and not quiet removal of part of the transmitted object.

Platform:

Additional context For context we initially found this with a misconfigured value from redux. A console.log(data) will produce the full object as expected.

eldir commented 3 weeks ago

Closing this here as I should have posted this in the client repo.

eldir commented 2 weeks ago

https://github.com/socketio/socket.io-client/issues/1627

darrachequesne commented 1 week ago

Hi! The library uses JSON.stringify() under the hood, which indeed removes functions (and undefined values as well).

Not sure what we could do here. Maybe with a custom parser?

Reference: https://socket.io/docs/v4/custom-parser/

eldir commented 1 week ago

Hi, I think there are a few good angles to mitigate this. Ultimately I wouldn't have expected it to necessarily process the function reference (even though a simple console.log(function_reference) will process a function reference into a JSON object). My confusion stems from it silently altering the schema by removing the key entirely, which feels like the worst possible option.

  1. setting key: undefined or key: error msg seem like a more natural way of processing this error
  2. throwing an exception on failed process would have been my instinct, but that will likely cause loads of downstream issues for projects that use the library and have accepted this behaviour
  3. throwing a warn on console could provide a less destructive path while allowing users to debug these issues

Maybe allowing a replacer function to be passed could be a solution? image