twilight
twilight copied to clipboard
twilight-rs
feat(lavalink)!: Lavalink refactor to v4 API
Here are the changes required to work against the new lavalink API: https://lavalink.dev/api/index.html.
I didn't fully implement all the API since some of it is new in v4. I only maintained the compatibility of the current system that was built on v3. I put the structs / enums under deserialization and serialization tests with json i received when testing the bot and with some in the documentation. I also tested against my bot and seems to work just as well as before.
Major changes were:
- REST API now for all outgoing events. I used a hyper http client for this.
- Websocket is only used for connection and incoming
- Structs and enums changed names and updated members
- New api now versions the api in the uri
/v4/... - V3 lavalink servers are not compatible now
- Refactored all of the incoming and outgoing into their own files within model folder
There are a few other things i want to change but they aren't critical. I want to clean up the http since it has some model structs that could make more sense in model. The http also just generates the body and parts for the http requests for some endpoints. I think the player needs to hold the client and make these calls for the user. There is too much detail exposed in my opinion. That will be a different day and effort but wanting to jot that down now.
Please let me know what I need to change and can do those and test with the basic bot. I updated the documentation with what i thought was relevant.
This covers the issue: #2192
@vilgotf
Please remove all unwraps that are not provably unreachable or document those functions as panicking on . I would also like struct fields and enum variants to be alphabetically sorted everywhere. Lastly, if it's not too much work, it would be great to complete the Lavalink v4 support in one go.
So for the panics, you just mean send them up as a result correct?
Sorry for the structs change. I was following the same order as the lavalink documentation to ensure i had all the new fields. I can revert those to alphabetical order. Didn't catch that before.
I personally don't have the time currently to finish the v4 implementation. I can maybe work on that at a later time but constrained at the moment. It will just take quite some time to verify the changes is the problem. The missing pieces are really just the extra stuff such as lavalink version, more filters (i haven't needed or used them), session info which isn't required, and i think route planning which i haven't needed either and quite frankly not sure how that all works. Unless someone else can help i won't be able to get that together.
So for the panics, you just mean send them up as a result correct?
Yes
I personally don't have the time currently to finish the v4 implementation. I can maybe work on that at a later time but constrained at the moment. It will just take quite some time to verify the changes is the problem. The missing pieces are really just the extra stuff such as lavalink version, more filters (i haven't needed or used them), session info which isn't required, and i think route planning which i haven't needed either and quite frankly not sure how that all works. Unless someone else can help i won't be able to get that together.
That's okay, We can defer the v4 additions to another PR.
@vilgotf I think I have address all the open items. Please let me know if i missed anything or you want further changes. I appreciate the feedback!
Also what is up with the book and docs jobs failing? I didn't touch any of the other ones and it seems to be broken. Not sure if there is a fix for these or not?
Also what is up with the book and docs jobs failing? I didn't touch any of the other ones and it seems to be broken. Not sure if there is a fix for these or not?
The book stuff is maybe fixed by #2326, I don't really know whats up with the docs though.
The only major issue I have remaining is TLS and HTTP2. Either we support it, or we don't. But having TLS features for websockets which are unused, and HTTP2 features for HTTP which are unused is a no go. My wild guess would be that 99% of people run lavalink without TLS, so we can just remove the features? On the other hand, TLS support is perhaps like 10 lines of code, so it might be best to keep it (and properly implement it!).
@vilgotf thank you for the help on updating the branch. Been super busy so haven't had a chance to respond here. I resolved your issues I believe.
@Gelbpunkt Can you be a bit more specific? I don't follow the comments. I think some of the confusion is from @vilgotf changes that were done. Please clarify how i should proceed. I would really like to close the PR out soon.
@vilgotf and @Gelbpunkt I know it has been a bit but would like to close this. I don't have much time lately so haven't made any progress. What needs to happen to close this?
I tried to change the ws to wss as suggested and that is a bigger change than we think. That is using the twilight-http crate which apparently doesn't support tls yet? I tried to swap the uri and it won't connect to the server. Please let me know how you would like to proceed. If we can merge this we can open up an issue for fixing the TLS / HTTP 2 parts here. I just don't have time to implement / figure it out and test it.
The deadline for v3 support is coming to a close soon (November 1, 2024)
I think the best way forward now is to rebase the PR and look over all pending, unresolved review comments to see which ones still apply or need clarification. I haven't replied to all of them in time in the past, but I'm happy to revisit them now to get this merged.
Regarding TLS, twilight-http plays no part in the websocket connection. You will need to add a parameter at node creation for whether TLS should be used in order to decide the scheme to use for connecting.
Thanks again for trying to get this merged, it's a big PR which makes this a bit more troublesome for everyone involved, but I think we're approaching the goal soon.
I appreciate the feedback @Gelbpunkt. If this TLS / HTTP2 is a deal breaker then I am going to close this PR. I don't have time to make this work. As i stated, that isn't supported right now and this is just a port over to v4 api. The TLS issue is a feature request in my opinion. I simply don't have time to develop and test this huge change. It isn't just a simple switch URL and be done.
Someone else can come through and pick that up. I won't close the branch on my fork so anyone should be able to clone that and use this as a starting point.
It isn't just a simple switch URL and be done.
Yes, it is exactly just that. If you're keen on dropping the PR I can implement it as well, it's not a big deal.
If you have time to help implement that then please do. Once you get it to a point you are happy, let me know and I can give it a test on my bot. More than happy to have you just push to this branch.
I can run through the changes once we get to a good point.