yjs / y-websocket

Websocket Connector for Yjs
https://docs.yjs.dev/ecosystem/connection-provider/y-websocket
MIT License
497 stars 256 forks source link

y-websocket reporting noisy error to console.error: 'Unable to compute message' #112

Closed diomedtmc closed 2 years ago

diomedtmc commented 2 years ago

Checklist

Describe the bug y-websocket reporting noisy error to console.error: 'Unable to compute message'

To Reproduce

  1. Set up some custom messages to use with y-websocket. Choose something well out of range from the typical messages types 1-4...maybe 101 or something.
  2. Establish a connection between client (using y-websocket) and a Server (using whatever).
  3. Have the server send the custom message, 101 (or whatever) to the connected client (which uses y-websocket)
  4. Observe the (noisy and misleading) console error logs in the client

Expected behavior y-websocket expects custom messages, as there is often an ecosystem around YJS documents that requires real-time access for the best possible experiences.

Environment Information

Additional context I can see the code that does it in y-websocket: https://github.com/yjs/y-websocket/blob/master/src/y-websocket.js#L78. It is worth noting, that in order to preserve the original y-websocket.ws.onmessage handler, we have to addEventListener, doing something like this: this.wsProvider.ws.addEventListener('message', onMessage);. The original implementation is so encapsulated that we cannot do much to re-route the messaging any other way (at least, as far as I can tell). We also have to construct our own decoder and peek the message buffer twice (once in our handler and once in the y-websocket one). We'd prefer the framework to factory out its encoders/decoders for us, and provide a suitable method to override which would expect custom message handling (possibly even with a boolean result to instruct the framework around us after inspecting the message type bytes). That said, simply removing the console error log would be sufficient.

dmonad commented 2 years ago

I suggest that you don't forward messages to y-websocket that y-websocket can't interpret. You can extend the protocol by adding more message types to the messagehandlers array: https://github.com/yjs/y-websocket/blob/master/src/y-websocket.js#L219

I don't want to turn off this error message because it does make sense to log unexpected cases.

diomedtmc commented 2 years ago

Oh, interesting. The lack of interface for what would seem to be a standard extension led me to believe there was no good way to do it…but I see the design now. Would you be opposed to making this a feature request for future framework improvements?

dmonad commented 2 years ago

Let's discuss the approach first before making a PR.

I'd accept a PR that adds a parameter for custom messages.

const customMessageHandlers = [
  {
    type: 128, // custom messages should have an id > 127 to leave room for "standard" messages
    handler: (decoder: decoding.Decoder) => ..
  } 
]
new WebsocketProvider(.., { customMessageHandlers })

These custom messages just have to be added to messageshandlers[typeid] = handler in the constructor.

diomedtmc commented 2 years ago

I was thinking a little different. First, I would love to export some TypeScript types/interfaces from this solution. The inline parameter types are great for calling a function right in-line, but the do very little to assist with separated concerns. Second, I should not know about your memory structure or storage. I know it is open source and TypeScript, but if you want to swap out that array for a map, I should not have to change my code. I would rather add a registration function (and unregister)? That accepts my message id and my handler. Validation can be provided then around the framework message types to prevent collision, etc...

What do you think?

(Note: Even with my changes, you can keep your current backing array for compatibility, but over time, you would usher people towards your interface and not your memory or storage)