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

Add room storage for Django Channels Consumers #1

Merged
merged 20 commits into from
Mar 27, 2024

Conversation

cacosandon
Copy link
Collaborator

@cacosandon cacosandon commented Mar 21, 2024

This PR introduces a shared storage for consumers within Django Channels.

Initially, each YjsConsumer had its unique YDoc, updating only from its client. This approach caused issues, such as consumers having different YDoc versions that made difficult to update to a persistent storage.

Now, by inheriting from BaseYRoomStorage, a unified storage can be utilized across all consumers and server-wide (e.g., in a Django Celery Job).

The storage's function, detailed in the documentation, is to manage document changes—fetching, updating, and saving them to a single source. An implementation example features Redis for ephemeral and Postgres for permanent storage.

Each user can define their own storage. Maybe using Django Cache, custom Redis or no storage at all (and just use it to broadcast updates from their connected clients). We just define the interface.

I would like to add some tests too 🤔 but not sure how.

Copy link

@zswaff zswaff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cacosandon incredible! Seems great to me. I have a variety of nits/suggestions but nothing too important on my end.

I was able to manually test a bit but we're still working on changes to the main ypy lib and those are blocking us from using this solution end to end. Once we get to that point we'll get this working in production, and in theory we might encounter some snags--I'll keep you posted. It seems like it will probably work though.

I hear you about automated tests here but I don't have any super concrete/brilliant suggestions, maybe someone on the main project will.

After you do whichever of my comments you agree with, you could just merge this and then open a new PR from this repo against the main fork and just copy in your description I guess?

ypy_websocket/django_channels/yjs_consumer.py Outdated Show resolved Hide resolved
ypy_websocket/django_channels/yjs_consumer.py Outdated Show resolved Hide resolved
ypy_websocket/django_channels/yjs_consumer.py Outdated Show resolved Hide resolved
ypy_websocket/django_channels/yjs_consumer.py Outdated Show resolved Hide resolved
ypy_websocket/django_channels/yjs_consumer.py Outdated Show resolved Hide resolved
ypy_websocket/django_channels/yroom_storage.py Outdated Show resolved Hide resolved
ypy_websocket/django_channels/yroom_storage.py Outdated Show resolved Hide resolved
ypy_websocket/django_channels/yroom_storage.py Outdated Show resolved Hide resolved
ypy_websocket/django_channels/yroom_storage.py Outdated Show resolved Hide resolved
@cacosandon
Copy link
Collaborator Author

@cacosandon incredible! Seems great to me. I have a variety of nits/suggestions but nothing too important on my end.

I was able to manually test a bit but we're still working on changes to the main ypy lib and those are blocking us from using this solution end to end. Once we get to that point we'll get this working in production, and in theory we might encounter some snags--I'll keep you posted. It seems like it will probably work though.

I hear you about automated tests here but I don't have any super concrete/brilliant suggestions, maybe someone on the main project will.

After you do whichever of my comments you agree with, you could just merge this and then open a new PR from this repo against the main fork and just copy in your description I guess?

Just applied all the changes you mentioned! Thanks for the review.

In a couple of hours I will test it with my codebase and if it's all good, then I will merge and create a PR in the original repo.

@cacosandon cacosandon merged commit 8814228 into main Mar 27, 2024
@cacosandon cacosandon deleted the feat/add-room-storage branch March 27, 2024 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants