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

Fixes #156 - Deprecate "shout" event #488

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xprazak2
Copy link

@xprazak2 xprazak2 commented Dec 22, 2020

Deprecate "shout" event for conversation channel
in favor of "message:created".

Issue

#156

@xprazak2
Copy link
Author

I think the removal can be done in 3 steps:

  1. Handle message:created and deprecate shout which keeps backward compatibility
  2. Migrate clients to new event name
  3. Remove shout in a future version when clients are migrated

Copy link
Collaborator

@reichert621 reichert621 left a comment

Choose a reason for hiding this comment

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

just one small nitpick, and could we also handle this in the notification_channel as well?

thanks!

# It is also common to receive messages from the client and
# broadcast to everyone in the current topic (conversation:lobby).
def handle_in("shout", payload, socket) do
defp handle_in_msg(event_name, payload, socket) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick -- can we name this something like handle_incoming_message instead? i think it's a bit more explicit :)

also let's move this to the bottom with the other private methods so we can fix the warning below 👍

Copy link
Author

Choose a reason for hiding this comment

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

I renamed the method and moved it to the bottom - naming is hard ;-)

I also made similar changes in notification_channel.

@xprazak2 xprazak2 force-pushed the deprecate-shout branch 4 times, most recently from 351569b to 2bfd0f6 Compare January 1, 2021 13:43
@@ -38,6 +38,22 @@ defmodule ChatApiWeb.NotificationChannelTest do
assert_push("shout", _msg)
end

test "message:created broadcasts to notification:lobby", %{
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like this test is failing?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Deprecate "shout" event for conversation channel
in favor of "message:created".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants