search

NicoNex / echotron

An elegant and concurrent library for the Telegram bot API in Go.
https://t.me/s/echotronnews
GNU Lesser General Public License v3.0
363 stars 26 forks source link

Feature/set custom httpserver #22

Closed sknr closed 2 years ago

sknr commented 2 years ago

Hi @NicoNex,

I was wondering if it will be better to pass a pointer to an http.Server instead of the http.Handler because then, the user of the your library has more control and can for example, initiate a gracefully server shutdown by himself. I know that this will bring braking changes, and I'm sad that I don't came up earlier with this idea, but maybe you can consider the change for the next major version. Thanks in advance.

NicoNex commented 2 years ago

Hi @sknr,

What's not clear to me is how the custom HTTP server is going to pass to the dispatcher the incoming updates. Does the user have to pass to the custom HTTP server the HandleWebhook dispatcher method for doing so?

sknr commented 2 years ago

You're absolutely right @NicoNex, the user has to pass the HandleWebhook to the server he will set on the dispatcher. I can write an example within the read.me file if you like, so it will be clear how to use it. But it doesn't change too much IMO, because right now, the user has handle the Webhook by himself, too.

NicoNex commented 2 years ago

Yes please, if it's not too much of a hassle for you a little example in the readme would be very nice and useful for the users.

sknr commented 2 years ago

@NicoNex please have a look at my last commit (c0a1158). I added the HandleWebhook automatically now so that the user does not have to care about registering the webhook himself.

sknr commented 2 years ago

@NicoNex I added an example. Feel free to adjust as needed ;)

NicoNex commented 2 years ago

@sknr from the example in the readme I see that you instantiate a new *http.ServeMux, wouldn't it get overridden once you call dsp.ListenWebhook(...) by the code in your commit c0a1158f5f7ba2e3296fc94c6ab61a6b7b5e95fa?

sknr commented 2 years ago

@NicoNex if you like the first approach better, let's revert the commit c0a1158 and I'll adjust the example within the README.md file.

NicoNex commented 2 years ago

@NicoNex Depending on the path defined in u.EscapedPath(), the currently registered path of the custom Handler might be overridden by the HandleWebhook method. This is on purpose so that the registered webhookUR, which was registered on the telegram servers, will be correctly processed by the HandleWebhook method. All other routes should stay intact. But maybe I overlooked something.

My bad you're absolutely right, I overlooked that in the commit c0a1158f5f7ba2e3296fc94c6ab61a6b7b5e95fa you save the user-defined multiplexer with mux.Handle("/", d.httpServer.Handler) before reassigning it with d.httpServer.Handler = mux, so your pr is totally ok for me and so is the example in the readme. I'll just slightly modify the example adding a good practice and then the pr will be merged.

Thanks again for the contribution and keep up with your awesome work.