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

Broadcast playlist edits in real time using websockets #1212

Merged
merged 13 commits into from Feb 10, 2021
Merged

Broadcast playlist edits in real time using websockets #1212

merged 13 commits into from Feb 10, 2021

Conversation

amCap1712
Copy link
Member

@amCap1712 amCap1712 commented Dec 27, 2020

Edits made by the owner or collaborators on a playlist are sent instantly to all users who are have the playlist opened at that time. This is an initial attempt that just works.

Things like login authentication, using a queue and fetching playlist in the server itself instead of sending multiple requests from the client so on can be explored and included if needed.

This PR also contains removing the follow server part. I needed that for easy local testing but for merging that can probably be split into a different PR. I also removed the UNIQUE_EXCHANGE queue from timescale_writer.py, as I was getting some errors. I think that was only being used by the follow server. I can try to explore that further if it is required.

Tasks left

  • Integration and unit tests are also required but I am currently not sure as to how to create those.
  • Review websockets setup once we remove the follow-server mechanism

@pep8speaks
Copy link

pep8speaks commented Dec 27, 2020

Hello @amCap1712! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-10 11:35:12 UTC

Copy link
Collaborator

@shivam-kapila shivam-kapila left a comment

Choose a reason for hiding this comment

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

Overall looks so good. Excited to see sockets in action :)

docker/docker-compose.yml Outdated Show resolved Hide resolved
docs/dev/devel-env.rst Outdated Show resolved Hide resolved
listenbrainz/websockets/webserver.py Outdated Show resolved Hide resolved
@amCap1712
Copy link
Member Author

PR's cleaned up :)

Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

It was really nice to see everything working as soon as I fired it up, from the WS container to the front-end updating automatically. Nicely done !

A couple of comments below but apart from that it's all working on the front-end side!

Comment on lines +150 to +156
handlePlaylistChange = (data: JSPFPlaylist): void => {
const newPlaylist = data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
handlePlaylistChange = (data: JSPFPlaylist): void => {
const newPlaylist = data;
handlePlaylistChange = (newPlaylist: JSPFPlaylist): void => {

listenbrainz/websockets/websockets.py Outdated Show resolved Hide resolved
listenbrainz/websockets/websockets.py Show resolved Hide resolved
@MonkeyDo
Copy link
Contributor

I'll make a note to point out that we (alastair, rob and I) discussed a future improvement where the API would directly react when a playlist is modified and have an update sent, rather than require that emitPlaylistChanged call from the front-end.

It won't be for right now anyway, and I think the feature already works well.

@amCap1712
Copy link
Member Author

the API would directly react when a playlist is modified and have an update sent, rather than require that emitPlaylistChanged call from the front-end

I too had thought about this. The way I thought we could get this to work is by setting up an update queue similar to what the TimescaleListenWriter does after successful inserts. It would be a bit more work, hence I decided to go with the current simpler implementation first.

@MonkeyDo
Copy link
Contributor

I too had thought about this. The way I thought we could get this to work is by setting up an update queue similar to what the TimescaleListenWriter does after successful inserts. It would be a bit more work, hence I decided to go with the current simpler implementation first.

I completely agree with the approach of Incremental improvements.
I think alastair and rob already have a plan to get the API to communicate with other parts of the project, but I honestly can't remember. 😊

amCap1712 and others added 7 commits January 13, 2021 20:05
This concerns the websocket playlist update mechanism.
We make sure we have set the state locally (after successful API calls) before calling emitPlaylistChanged as a callback, since it relies on that updated state.
@MonkeyDo
Copy link
Contributor

@brainzbot retest this please

@MonkeyDo MonkeyDo self-requested a review February 5, 2021 12:57
Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Looks good, thanks !

We use a single WEBSOCKETS_SERVER_URL now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants