jupyter-server / pycrdt-websocket

WebSocket Connector for pycrdt
https://jupyter-server.github.io/pycrdt-websocket
MIT License
14 stars 10 forks source link

get_room should not create a room, since YRoom might be subclassed #34

Open Zsailer opened 6 months ago

Zsailer commented 6 months ago

Right now, get_room will create an instance of a YRoom if the room doesn't exist. This is problematic for libraries that subclass YRoom, e.g. jupyter-collaboration uses it's own DocumentRoom subclass. The room is created and added to the Websocket Server using the add_room API. This can lead to an unintentional creation of a mix of YRoom and DocumentRoom instances.

welcome[bot] commented 6 months ago

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively. welcome You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

davidbrochart commented 6 months ago

Thanks @Zsailer. The get_room() method in pycrdt-websocket is used to serve a WebSocket client. If the room doesn't exist yet, one must be created. A subclass of WebsocketServer must ensure its custom room is created before serving the WebSocket. But in the general case where WebsocketServer is not subclassed, we need to have a default way of creating a room. But I understand your point, and maybe we should have a create_room() in pycrdt-websocket, that would create a generic room by default, that WebsocketServer subclasses could override with their custom implementation?