shipgirlproject / Shoukaku

A stable, powerful and updated wrapper around Lavalink
https://guide.shoukaku.shipgirl.moe/
MIT License
287 stars 86 forks source link

V2 refactor #42

Closed Deivu closed 3 years ago

Deivu commented 3 years ago

This is the WIP update for Shoukaku, this aims to fix and reduce the clutter of v1 while providing the same benefits of v1

Progress: [x] Code [x] Testing [x] Documentation [x] Typescript Typings

For discussions or recommendations, feel free to comment down in this thread

thrace19 commented 3 years ago

Some fixes:

  1. https://github.com/Deivu/Shoukaku/blob/next/src/guild/ShoukakuConnection.js#L140 - It doesn't work and returns a Promise instead of Void
  2. https://github.com/Deivu/Shoukaku/blob/next/src/node/ShoukakuSocket.js#L51 - Returns 0, even the bot is connected
  3. thrace19 commented 3 years ago

    Yeah and maybe don't forget this https://github.com/Deivu/Shoukaku/issues/38

    Gaeta commented 3 years ago

    Any ETA on v2 refactor?

    Deivu commented 3 years ago

    No ETA for now, most likely when Lavalink dev goes to stable, and Discord.JS v13 gets finalized in its breaking changes, though it also depends on how fast I work, or when I will resume coding again

    Deivu commented 3 years ago

    @inhydrox state should now be turning to connected in voice connection, as for moveChannel, I dropped it as I don't think I need to support that feature with Shoukaku as this deals with Discord REST which is not something I want to support with Shoukaku

    itsmishra19 commented 3 years ago

    @inhydrox state should now be turning to connected in voice connection, as for moveChannel, I dropped it as I don't think I need to support that feature with Shoukaku as this deals with Discord REST which is not something I want to support with Shoukaku

    Alright, I'll test the voice connection one and let you know if I found any more bugs.

    kakarot-dev commented 3 years ago

    Hmmm pr ded

    0t4u commented 3 years ago

    Types for next are in PR (#51)

    0t4u commented 3 years ago

    So um my computer borked don’t expect updates lmfao

    Deivu commented 3 years ago

    @0t4u there have been changes, quite a bit so typings may need to change, just do it at your own pace, as the recent commits enables the lib to be used on other libraries with ease

    0t4u commented 3 years ago

    @Deivu updated. Please check if it works.

    kakarot-dev commented 3 years ago
    0t4u commented 3 years ago

    Lol the amount of commits

    kakarot-dev commented 3 years ago

    Is this pr ready for testing :octocat:

    kakarot-dev commented 3 years ago

    Lol the amount of commits

    Yea, sorry bout that, i used mobile and couldnt use my laptop xd won’t happen again

    Deivu commented 3 years ago

    It could be tested, but there might be changes like changing things to camelcase one I decided to do so

    kakarot-dev commented 3 years ago

    ~~Oh, can i do that ill use laptop this time~~ nvm ill do it if anyone else isnt doing it

    Deivu commented 3 years ago

    This is ready for testing and review, in order for this to get merged, First, We need to wait for Discord.JS v13 to be the current stable, and Secondly, This needs to be tested thoroughly to ensure this one won't cause issues, specially on bots in production.

    kakarot-dev commented 3 years ago

    Discord js v13 will be released when crawl wakes up. ( im not lying, he announced that in the announcement channel )

    Deivu commented 3 years ago

    Discord js v13 will be released when crawl wakes up. ( im not lying, he announced that in the announcement channel )

    The second one must be also be considered, which is a through test of the next refactor, I don't want bots breaking in production due to unforseen bugs in the library

    noxzym commented 3 years ago

    Discord js v13 will be released when crawl wakes up. ( im not lying, he announced that in the announcement channel )

    and need node v16 image

    Deivu commented 3 years ago

    Engine is updated

    kakarot-dev commented 3 years ago

    readme need to be fixed to show that node 16 is required

    kakarot-dev commented 3 years ago

    Types of ShoukakuTrack and ShoukakuTrackList arent there

    Deivu commented 3 years ago

    Types of ShoukakuTrack and ShoukakuTrackList arent there @0t4u Could you add this typings

    kakarot-dev commented 3 years ago

    if you want i could add them, i just need to know what “raw” will be

    0t4u commented 3 years ago

    @Deivu gimme a few hrs

    Deivu commented 3 years ago

    if you want i could add them, i just need to know what “raw” will be

    Tracklist raw is this

    {
      "loadType": "TRACK_LOADED",
      "playlistInfo": {},
      "tracks": [
        {
          "track": "QAAAjQIAJVJpY2sgQXN0bGV5IC0gTmV2ZXIgR29ubmEgR2l2ZSBZb3UgVXAADlJpY2tBc3RsZXlWRVZPAAAAAAADPCAAC2RRdzR3OVdnWGNRAAEAK2h0dHBzOi8vd3d3LnlvdXR1YmUuY29tL3dhdGNoP3Y9ZFF3NHc5V2dYY1EAB3lvdXR1YmUAAAAAAAAAAA==",
          "info": {
            "identifier": "dQw4w9WgXcQ",
            "isSeekable": true,
            "author": "RickAstleyVEVO",
            "length": 212000,
            "isStream": false,
            "position": 0,
            "title": "Rick Astley - Never Gonna Give You Up",
            "uri": "https://www.youtube.com/watch?v=dQw4w9WgXcQ"
          }
        }
      ]
    }

    ShoukakuTrack raw is the object inside the array of tracks

    kakarot-dev commented 3 years ago

    if you want i could add them, i just need to know what “raw” will be

    Tracklist raw is this

    {
      "loadType": "TRACK_LOADED",
      "playlistInfo": {},
      "tracks": [
        {
          "track": "QAAAjQIAJVJpY2sgQXN0bGV5IC0gTmV2ZXIgR29ubmEgR2l2ZSBZb3UgVXAADlJpY2tBc3RsZXlWRVZPAAAAAAADPCAAC2RRdzR3OVdnWGNRAAEAK2h0dHBzOi8vd3d3LnlvdXR1YmUuY29tL3dhdGNoP3Y9ZFF3NHc5V2dYY1EAB3lvdXR1YmUAAAAAAAAAAA==",
          "info": {
            "identifier": "dQw4w9WgXcQ",
            "isSeekable": true,
            "author": "RickAstleyVEVO",
            "length": 212000,
            "isStream": false,
            "position": 0,
            "title": "Rick Astley - Never Gonna Give You Up",
            "uri": "https://www.youtube.com/watch?v=dQw4w9WgXcQ"
          }
        }
      ]
    }

    ShoukakuTrack raw is the object inside the array of tracks

    Ty ty! I’ll do it

    0t4u commented 3 years ago

    @Deivu @kakarot-dev it’s already in the Constants.d.ts file

    itsmishra19 commented 3 years ago

    Huh check the constants files in types folder

    itsmishra19 commented 3 years ago

    It is there

    Deivu commented 3 years ago

    @Deivu @kakarot-dev it’s already in the Constants.d.ts file

    smh I got jeibaited there

    kakarot-dev commented 3 years ago

    Oh, i got confused too when i didnt see the file xd

    itsmishra19 commented 3 years ago

    smh

    kakarot-dev commented 3 years ago

    One question tracklist has the property “playlistName” it also has “selectedTrack” (https://github.com/Deivu/Shoukaku/blob/5a2ab750545ccd17d58aae741966fe9aea1f8f1e/types/Constants.d.ts#L38) while the example of @Deivu has “playlistInfo” so…. Is that intentional?

    Deivu commented 3 years ago

    My structure don't have playlistInfo, instead its a property directly in the class for convenience https://github.com/Deivu/Shoukaku/blob/next/src/struct/ShoukakuTrackList.js

    kakarot-dev commented 3 years ago

    Oh, kk understood

    kakarot-dev commented 3 years ago

    Someone mentioned me?

    kakarot-dev commented 3 years ago

    Code tested in production and works perfectly.

    Allvaa commented 3 years ago

    when setting volume to 0 using setFilters, its value will be always 1. because OR operator checks its boolean, and yeah 0 is false, so it will pick the 1.0 instead https://github.com/Deivu/Shoukaku/blob/c45294fcc2966b114bbf8a9c1d37263f68c76656/src/struct/ShoukakuFilter.js#L51 maybe we can replace the OR operator above with nullish coalescing operator (??) ?

    kakarot-dev commented 3 years ago

    normally you would just use the mute method to remove voice, so yea....

    Allvaa commented 3 years ago

    normally you would just use the mute method to remove voice, so yea....

    indeed, but it's still a bug tho. the function doesn't set the correct value of what is given

    Deivu commented 3 years ago

    when setting volume to 0 using setFilters, its value will be always 1. because OR operator checks its boolean, and yeah 0 is false, so it will pick the 1.0 instead https://github.com/Deivu/Shoukaku/blob/c45294fcc2966b114bbf8a9c1d37263f68c76656/src/struct/ShoukakuFilter.js#L51

    maybe we can replace the OR operator above with nullish coalescing operator (??) ?

    Makes sense for it to be on ?? instead of ||, updated in latest commit

    Allvaa commented 3 years ago

    when setting volume to 0 using setFilters, its value will be always 1. because OR operator checks its boolean, and yeah 0 is false, so it will pick the 1.0 instead https://github.com/Deivu/Shoukaku/blob/c45294fcc2966b114bbf8a9c1d37263f68c76656/src/struct/ShoukakuFilter.js#L51

    maybe we can replace the OR operator above with nullish coalescing operator (??) ?

    Makes sense for it to be on ?? instead of ||, updated in latest commit

    thanks. anyway will sourceName be added to ShoukakuTrack#info?

    Deivu commented 3 years ago

    thanks. anyway will sourceName be added to ShoukakuTrack#info?

    Any relevant info on what git commit this was merged?

    Allvaa commented 3 years ago

    thanks. anyway will sourceName be added to ShoukakuTrack#info?

    Any relevant info on what git commit this was merged?

    you mean merged on dev branch? if so this is the commit https://github.com/freyacodes/Lavalink/commit/626c2186641d249e472a1070bdc61a99c077e6ba

    Deivu commented 3 years ago

    I think this is mostly ironed out, feel free to comment if it's stable on your end or not

    kakarot-dev commented 3 years ago

    its stable for me 👍