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

sending/publishing data with headers #487

Closed
Polve opened this issue Apr 11, 2024 · 7 comments
Closed

sending/publishing data with headers #487

Polve opened this issue Apr 11, 2024 · 7 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@Polve
Copy link

Polve commented Apr 11, 2024

Problem / use case

I'm trying to port a simple stomp client developed in TS using the stompjs library.

When I write to the server I need to pass special headers data (for handling RPC for example), and I use the Client.publish method defined here: https://stomp-js.github.io/api-docs/latest/classes/Client.html#publish

With krossbow it's not clear how to pass/add custom headers when calling send* functions: is there a way?

Feature (solution)

Ideally I can optionally pass additional headers to the send* functions in order to give some more metadata to the server.

Current alternatives

No response

@Polve Polve added the enhancement New feature or request label Apr 11, 2024
@joffrey-bion
Copy link
Owner

joffrey-bion commented Apr 11, 2024

Hi! Thanks for reaching out. You might currently be using the send() extension function that takes a String as destination parameter. If you need more headers than just the destination, you may use another overload (send, convertAndSend, or convertAndSend depending on your case) that takes a StompSendHeaders object.

You can create such header object with your destination and custom headers map:

val headers = StompSendHeaders(
    destination = "/your-destination",
    customHeaders = mapOf("The-Header" to "value"),
)
val body = ...
session.send(headers, body)

@joffrey-bion
Copy link
Owner

joffrey-bion commented Apr 11, 2024

But it's true that there is no overload of sendText, sendBinary etc. that takes a StompSendHeaders parameter. It could definitely be added if there is demand for it.

@Polve
Copy link
Author

Polve commented Apr 11, 2024

For now... there is my demand ❤️ for two reasons:

  • it's nice to try have similar API between implementations
  • it's not immediately obvious that a StompSendHeaders includes the queue name (and must be included)

So having the possibility to have convenience methods where you can add optional headers seems nice.

@joffrey-bion
Copy link
Owner

For now... there is my demand

Is your demand to have a sendText(headers, textBody) extension? Or something more like sendText(destination, customHeaders, textBody)?

it's nice to try have similar API between implementations

It might sound nice, but this is not generally a design goal (in most libraries I know at least). Different ecosystems have different conventions, so even when implementing the same specification, the design may vary to be more idiomatic. Even within the same ecosystem, different libraries choose different designs. For example, there are many HTTP client libraries in the JVM ecosystem, and they don't have the same API designs (and this is for the better).

it's not immediately obvious that a StompSendHeaders includes the queue name (and must be included)

I understand this might not be obvious initially, but this is simply how the STOMP protocol is designed. The destination is just one of the standard STOMP headers, like the others. Krossbow respects this in its "raw" APIs: it provides a way to specify the headers and body, as defined in the protocol, in a type safe way.

That said, because the destination is the only mandatory header, it is very common to send just this one. This is why Krossbow makes it a bit more convenient and provides overloads/extension functions with just the destination.
Similarly, there are overloads for sending text and binary bodies because those are the only 2 options and it makes the API less cumbersome.

Now, we have to draw the line somewhere and not multiply the API unnecessarily. This is why we don't provide all possible combinations right now (with body conversions, there are 3 times more functions).

The idea is to provide extra convenience for the use cases that come up often. Hence why I'm saying we could consider adding such overloads, but there is a high maintenance load for this addition, so we must consider how common this use case is.

If you personally have a case with a specific set of headers that come up often, including custom headers, it's actually better to create your own extension function with type safe arguments for your custom headers:

suspend fun StompSession.sendText(
    destination: String,
    myHeader1: String, // could be another type
    myHeader2: String,
    body: String,
) {
    val headers = StompSendHeaders(
        destination = destination,
        customHeaders = mapOf(
            "My-Header-1" to myHeader1,
            "My-Header-2" to myHeader2,
        ),
     )
    send(headers, FrameBody.Text(body))
}

And in this case it doesn't matter much whether Krossbow provides the convenience function or not, because you would only use it once to define your own extension anyway.

I hope this clarified a bit why this is not already there, and why I'm digging a bit first before making any additions :)

@Polve
Copy link
Author

Polve commented Apr 19, 2024

Thanks for the detailed explanation, very much appreciated, and I agree with everything you said.

So I now have another request: handling automatic reconnection in case of connection loss (most of times intermediate proxys as cloudflare just reset the session for various reasons and we can reconnect just immediately).

This kind of functionality is provided by the stompjs npm library and it's very handy together with the lifecycle events to handle them (a new app setup may be needed after reconnection).

Before opening a new issue about this I would like to discuss here to understand if it makes sense to you.

@joffrey-bion
Copy link
Owner

@Polve reconnection totally makes sense indeed, and this is already planned: #102

I couldn't find the time to handle this so far, but I definitely know this is important, so stay tuned 😄

@Polve
Copy link
Author

Polve commented Apr 19, 2024

Ah, I missed that one, thanks!

I think we can close this issue now.

@Polve Polve closed this as completed Apr 19, 2024
@joffrey-bion joffrey-bion added the question Further information is requested label Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants