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

feat: implement first rough mvp #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: implement first rough mvp #1

wants to merge 1 commit into from

Conversation

iCrawl
Copy link
Member

@iCrawl iCrawl commented Jun 17, 2021

No description provided.

@iCrawl iCrawl requested review from amishshah and kyranet June 17, 2021 17:43
example/package.json Outdated Show resolved Hide resolved
Copy link
Member

@amishshah amishshah left a comment

Choose a reason for hiding this comment

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

I looked mainly at the @discordjs/voice-specific code but that looks good to me so far 🚀

try {
await entersState(this.voiceConnection, VoiceConnectionStatus.Ready, 20_000);
} catch {
if (this.voiceConnection.state.status !== VoiceConnectionStatus.Destroyed) {
this.voiceConnection.destroy();
}
} finally {
this.readyLock = false;
}
Copy link
Member

Choose a reason for hiding this comment

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

We need to remember to change this at some point, because at the moment it prevents a 5 rejoin attempt limit and replaces it with a 20 second hard cap.

@Nytelife26
Copy link

There's a few issues with this. While an interesting concept, youtube-dl is, as you know, in Python, and effectively just offers a layer over ffmpeg. It would be more efficient to use ffmpeg directly, rather than run both Node and a Python environment on the system - especially when voicelink would not need a large amount of youtube-dl's functionality to operate.

Just my take.

package.json Outdated Show resolved Hide resolved
@Nytelife26

This comment has been minimized.

@iCrawl iCrawl force-pushed the feat/first-mvp branch 4 times, most recently from f3e3a86 to 5c92a14 Compare July 6, 2021 14:41
@iCrawl iCrawl requested a review from kyranet July 6, 2021 18:36
Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

This plus @amishshah's review 19 days ago.

Comment on lines +49 to +50
const clientId = req.params.clientId;
const guildId = req.params.guildId;
Copy link
Member

Choose a reason for hiding this comment

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

Why not something like this?

Suggested change
const clientId = req.params.clientId;
const guildId = req.params.guildId;
const { clientId, guildId } = req.params;

Comment on lines +72 to +73
// @ts-ignore
const { entries } = await youtubedl(`ytsearch10:${search}`, { dumpSingleJson: '' });
Copy link
Member

Choose a reason for hiding this comment

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

Why is it ignored? What's the 10 in the search? Is it not possible to search in SoundCloud as well like Lavalink does?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. The types are wrong/don't support dumpSingleJson.
  2. n refers to the number of results wanted.
  3. Probably could, but I wanted to avoid the user doing ytsearchN themselves, so there needs to be a nicer way to handle this then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants