Skip to content

Allow receiving all messages with a non-null 'status' #100

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

Conversation

ccolorcat
Copy link
Contributor

In addition to the status "ok", "error" and "timeout", there may be custom status such as "approved".
It would be more convenient if all messages with a non-null "status" could be received in the same callback.

@dsrees
Copy link
Owner

dsrees commented Nov 30, 2023

This seems like a deviation of the API set out by the phoenix JS client. I don't intend to add anything to these libraries that doesn't exist in the official JS client, unless it is required because of operating inside of a JVM environment instead of a JS environment.

AFAIK, these "ok" and "error" status are from the phoenix framework. I'm not aware of being able to provide a custom statuses. Could you point to specific phoenix documentation/examples that would support this functionality?

@ccolorcat
Copy link
Contributor Author

ccolorcat commented Dec 1, 2023

I totally agree with your points about the phoenix API.

But in actual projects, the backend may customize some status. For example, in one of our previous projects, messages sent by users were automatically reviewed. If the review passes, the status is "approved".

Adding this method will make it easier to use with Kotlin's coroutines. When I send a message I expect to receive a response regardless of whether the response status is "ok" or "error" or any other custom status. Combined with Kotlin's coroutine, we can convert this response into a method similar to synchronous reception, such as:

    suspend fun push(event: String, payload: Payload): Reply {
        return suspendCancellableCoroutine { cc ->
            channel.push(event, payload).receiveAll { status, message ->
                if (cc.isActive) {
                    cc.resume(Reply(status, message.payload))
                }
            }
        }
    }

So, even without custom status, this API provides more possibilities for extension. When we don't know how many status there are and don't provide this API, it becomes more difficult to implement the above function.

In my opinion, all the API defined by Phoenix JS Client should be implemented, but it does not stipulate that we cannot extend its undefined API.

Copy link
Owner

@dsrees dsrees left a comment

Choose a reason for hiding this comment

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

Thank you for providing a thorough explanation. I see the benefit to this approach now. One day, I'd like to make the library more coroutine friendly but for now I think this is a good step.

I've requested a change regarding the function name and additional documentation. After that, I can merge this and get a release out for you

@@ -117,6 +120,18 @@ class Push(
return this
}

fun receiveAll(callback: (status: String, message: Message) -> Unit): Push {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested documentation and function name.

    /**
    * Receives any event that was a response to an outbound message.
    *
    * Example:
    *    channel
    *        .send("event", mPayload)
    *        .receive { status, message ->
    *            print(status) // "ok"
    *        }
    */
    fun receive(callback: (status: String, message: Message) -> Unit): Push {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot, I've changed the method name and comments based on your comment.

@ccolorcat ccolorcat force-pushed the feature/allow-receiving-all-messages-with-status branch from f23b37d to 3c7d3ed Compare December 4, 2023 02:22
In addition to the status "ok", "error" and "timeout", there may be
custom status such as "approved".
It would be more convenient if all
messages with a non-null "status" could be received in the same
callback.
@ccolorcat ccolorcat force-pushed the feature/allow-receiving-all-messages-with-status branch from 3c7d3ed to b60c338 Compare December 4, 2023 02:32
@ccolorcat ccolorcat requested a review from dsrees December 4, 2023 06:45
@dsrees dsrees merged commit 60d3e49 into dsrees:master Dec 4, 2023
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