Skip to content

Commit

Permalink
fix(engines): remove unnecessary array for search method (#17)
Browse files Browse the repository at this point in the history
BREAKING CHANGE: Changed type `PlayerEngine` and all pre-built engines
(youtube, spotify and file) to return `SearchResult|null` instead of
`SearchResult[]`. The previous return value just wrapped the search
result into an array so its length was always 1. Thats why it has been
changed.
BREAKING CHANGE: `player.search()` now returns `SearchResult|null`
instead of `SearchResult[]`
  • Loading branch information
larsrickert authored May 13, 2023
1 parent 48fb2cc commit 503ab0b
Show file tree
Hide file tree
Showing 13 changed files with 101 additions and 131 deletions.
18 changes: 9 additions & 9 deletions docs/api/core.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,12 @@ Immediate plays the first of the given tracks, skips current track if playing. T
// get your voice channel here, e.g. from a slash command
const voiceChannel;

const searchResults = await player.search("Luis Fonsi - Despacito");
if (searchResults.length && searchResults[0].tracks.length) {
const searchResult = await player.search("Luis Fonsi - Despacito");
if (searchResult?.tracks.length) {
await player.play({
channel: voiceChannel,
// play first matched song for Despacito
tracks: searchResults[0].tracks.slice(1),
tracks: searchResult.tracks[0],
});
}
```
Expand All @@ -167,11 +167,11 @@ Adds the given tracks to the end of the queue. Immediately plays first track in
// get your voice channel here, e.g. from a slash command
const voiceChannel;

const searchResults = await player.search("Some YouTube Playlist");
if (searchResults.length && searchResults[0].tracks.length) {
const searchResult = await player.search("Some YouTube Playlist");
if (searchResult?.tracks.length) {
await player.add({
channel: voiceChannel,
tracks: searchResults[0].tracks,
tracks: searchResult.tracks,
});
}
```
Expand Down Expand Up @@ -411,12 +411,12 @@ Searches tracks for the given query.
- **Example**

```ts
const searchResults = await player.search("Luis Fonsi - Despacito");
if (searchResults.length) {
const searchResult = await player.search("Luis Fonsi - Despacito");
if (!searchResult?.tracks.length) {
console.log("No search results found for Despacito");
} else {
console.log(
`Found ${searchResults[0].tracks.length} matching tracks for Despacito`
`Found ${searchResult.tracks.length} matching tracks for Despacito`
);
}
```
Expand Down
2 changes: 1 addition & 1 deletion docs/guide/engines.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Player engines are the heart of `discord-player-plus`. They are responsible for
When you use `player.search()` the engine will be detected automatically but you can also specify which engine you want to use with:

```ts
const searchResults = await player.search("my song to search", {
const searchResult = await player.search("my song to search", {
source: "spotify",
});
```
Expand Down
6 changes: 3 additions & 3 deletions docs/guide/getting-started.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,12 +127,12 @@ const player = playerManager.get("my-guild-id");
// get your voice channel here, e.g. from a slash command
const voiceChannel;

const searchResults = await player.search("Luis Fonsi - Despacito");
if (searchResults.length && searchResults[0].tracks.length) {
const searchResult = await player.search("Luis Fonsi - Despacito");
if (searchResult?.tracks.length) {
await player.play({
channel: voiceChannel,
// play first matched song for Despacito
tracks: searchResults[0].tracks.slice(1),
tracks: searchResult.tracks[0],
});
}
```
40 changes: 15 additions & 25 deletions src/__tests__/engines/file.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { IAudioMetadata, parseFile } from "music-metadata";
import path from "path";
import { afterEach, describe, expect, it, Mock, test, vi } from "vitest";
import { Mock, afterEach, describe, expect, it, test, vi } from "vitest";
import { fileEngine } from "../../engines/file";
import { Track } from "../../types/engines";

Expand Down Expand Up @@ -79,20 +79,15 @@ describe("file engine", () => {

it("searches track with metadata", async () => {
// should return no result when fileRoot is not set
let searchResults = await fileEngine.search(expectedTrack.url, {});
expect(searchResults.length).toBe(0);
let searchResult = await fileEngine.search(expectedTrack.url, {});
expect(searchResult).toBe(null);

// should return result when fileRoot is set
searchResults = await fileEngine.search(expectedTrack.url, { fileRoot });
const result = searchResults[0];
expect(result).toBeDefined();
expect(result.playlist).toBeUndefined();
expect(result.source).toBe("file");
expect(result.tracks.length).toBe(1);

const track = result.tracks[0];
expect(track).toBeDefined();
expect(track).toEqual(expectedTrack);
searchResult = await fileEngine.search(expectedTrack.url, { fileRoot });
expect(searchResult).toStrictEqual({
source: "file",
tracks: [expectedTrack],
});
});

it("searches track without metadata", async () => {
Expand All @@ -106,25 +101,20 @@ describe("file engine", () => {
source: "file",
};

let searchResults = await fileEngine.search(expectedTrack.url, {
let searchResult = await fileEngine.search(expectedTrack.url, {
fileRoot,
});
const result = searchResults[0];
expect(result).toBeDefined();
expect(result.playlist).toBeUndefined();
expect(result.source).toBe("file");
expect(result.tracks.length).toBe(1);

const track = result.tracks[0];
expect(track).toBeDefined();
expect(track).toEqual(expectedTrack);
expect(searchResult).toEqual({
source: "file",
tracks: [expectedTrack],
});

// handles metadata error
(parseFile as Mock).mockImplementation(() => {
throw Error("This should be handled");
});
searchResults = await fileEngine.search(expectedTrack.url, { fileRoot });
expect(searchResults.length).toBe(0);
searchResult = await fileEngine.search(expectedTrack.url, { fileRoot });
expect(searchResult).toBe(null);
});

it("gets stream", async () => {
Expand Down
54 changes: 25 additions & 29 deletions src/__tests__/engines/spotify.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { YouTubeStream } from "play-dl";
import spotify, { Tracks } from "spotify-url-info";
import { afterEach, describe, expect, it, vi } from "vitest";
import { mock } from "vitest-mock-extended";
import { spotifyEngine, SpotifyPlaylist } from "../../engines/spotify";
import { SpotifyPlaylist, spotifyEngine } from "../../engines/spotify";
import { youtubeEngine } from "../../engines/youtube";
import { Playlist, Track } from "../../types/engines";

Expand Down Expand Up @@ -71,64 +71,60 @@ describe.concurrent("spotify engine", () => {
});

it.each(["https://open.spotify.com/"])("is responsible for %s", (query) => {
let isResponsible = spotifyEngine.isResponsible(query);
let isResponsible = spotifyEngine.isResponsible(query, {});
expect(isResponsible).toBe(true);

isResponsible = spotifyEngine.isResponsible(
query.replace("https://", "http://")
query.replace("https://", "http://"),
{}
);
expect(isResponsible).toBe(true);
});

it("should search track", async () => {
const result = await spotifyEngine.search(expectedTrack.url, {}, {});
expect(result.length).toBe(1);
expect(result[0].source).toBe(spotifyEngine.source);
expect(result[0].tracks.length).toBe(1);
expect(result[0].tracks[0]).toEqual(expectedTrack);
expect(result[0].playlist).toBeUndefined();
const searchResult = await spotifyEngine.search(expectedTrack.url, {}, {});
expect(searchResult).toEqual({
source: "spotify",
tracks: [expectedTrack],
});
});

it("searches playlist", async () => {
const searchResults = await spotifyEngine.search(
const searchResult = await spotifyEngine.search(
expectedPlaylist.url,
{},
{}
);
const result = searchResults[0];
expect(result).toBeDefined();
expect(result.tracks.length).toBeGreaterThanOrEqual(1);
expect(result.source).toBe("spotify");
expect(result.playlist).toBeDefined();
expect(result.tracks.length).toBe(100);
expect(result.playlist).toEqual(expectedPlaylist);

result.tracks.forEach((track) => {
expect(track.playlist).toBeDefined();
expect(track.playlist).toEqual(expectedPlaylist);
expect(track.playlist).toBe(result.playlist);

expect(searchResult).not.toBeNull();
expect(searchResult?.source).toBe("spotify");
expect(searchResult?.tracks.length).toBe(100);
expect(searchResult?.playlist).toStrictEqual(expectedPlaylist);

searchResult?.tracks.forEach((track) => {
expect(track.playlist).toBe(searchResult?.playlist);
});
});

it("searches playlist with limit", async () => {
const limit = 64;
const searchResults = await spotifyEngine.search(
const searchResult = await spotifyEngine.search(
"https://open.spotify.com/playlist/3xMQTDLOIGvj3lWH5e5x6F",
{},
{ limit }
);

const result = searchResults[0];
expect(result).toBeDefined();
expect(result.tracks.length).toBe(limit);
expect(searchResult).not.toBeNull();
expect(searchResult?.tracks.length).toBe(limit);
});

it("gets stream", async () => {
const youtubeSearchSpy = vi
.spyOn(youtubeEngine, "search")
.mockResolvedValue([
{ source: youtubeEngine.source, tracks: [expectedTrack] },
]);
.mockResolvedValue({
source: youtubeEngine.source,
tracks: [expectedTrack],
});
const youtubeStreamSpy = vi
.spyOn(youtubeEngine, "getStream")
.mockResolvedValue(mock<YouTubeStream>());
Expand Down
30 changes: 13 additions & 17 deletions src/__tests__/engines/youtube.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,26 +47,24 @@ describe.concurrent("youtube engine", () => {
});

test("should not return search results for empty query", async () => {
const results = await youtubeEngine.search(" ", {}, {});
expect(results).toStrictEqual([]);
const result = await youtubeEngine.search(" ", {}, {});
expect(result).toBeNull();
});

test("should return search results for video query", async () => {
const searchSpy = vi.spyOn(playDl, "search").mockResolvedValue([mockVideo]);

const query = "https://www.youtube.com/watch?v=testVideoId";

const results = await youtubeEngine.search(query, {}, {});
const result = await youtubeEngine.search(query, {}, {});

expect(searchSpy).toHaveBeenCalledWith(query, {
source: { youtube: "video" },
});
expect(results).toStrictEqual([
{
source: "youtube",
tracks: [expectedTrack],
},
]);
expect(result).toStrictEqual({
source: "youtube",
tracks: [expectedTrack],
});
});

test("should return search results for playlist query", async () => {
Expand All @@ -85,7 +83,7 @@ describe.concurrent("youtube engine", () => {

const query = "https://www.youtube.com/playlist?list=testPlaylistId";

const results = await youtubeEngine.search(query, {}, {});
const result = await youtubeEngine.search(query, {}, {});

const expectedPlaylist: Playlist = {
title: "testPlaylistTitle",
Expand All @@ -96,13 +94,11 @@ describe.concurrent("youtube engine", () => {
expect(validateSpy).toHaveBeenCalledWith(query);
expect(playlistInfoSpy).toHaveBeenCalledWith(query, { incomplete: true });

expect(results).toStrictEqual([
{
source: "youtube",
playlist: expectedPlaylist,
tracks: [{ ...expectedTrack, playlist: expectedPlaylist }],
},
]);
expect(result).toStrictEqual({
source: "youtube",
playlist: expectedPlaylist,
tracks: [{ ...expectedTrack, playlist: expectedPlaylist }],
});
});

test("searches tracks with limit", async () => {
Expand Down
11 changes: 5 additions & 6 deletions src/commands/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,8 @@ export async function playTracks(

// search tracks
const searchResult = await player.search(query);
const firstResult = searchResult[0];

if (!firstResult || !firstResult.tracks.length) {
if (!searchResult?.tracks.length) {
await interaction.followUp({
content: playerManager.translations.play.noTracksFound.replace(
"{query}",
Expand All @@ -119,14 +118,14 @@ export async function playTracks(

// play track(s)
const playOptions: PlayOptions = {
tracks: firstResult.playlist
? firstResult.tracks
: firstResult.tracks.slice(0, 1),
tracks: searchResult.playlist
? searchResult.tracks
: searchResult.tracks.slice(0, 1),
channel: interaction.member.voice.channel,
};

if (immediate) await player.play(playOptions);
else await player.add(playOptions);

return firstResult;
return searchResult;
}
5 changes: 2 additions & 3 deletions src/commands/insert.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,8 @@ export const createInsertCommand: CreateCommandFunc = (

// search tracks
const searchResult = await player.search(query);
const firstResult = searchResult[0];

if (!firstResult || !firstResult.tracks.length) {
if (!searchResult?.tracks.length) {
await interaction.followUp({
content: playerManager.translations.play.noTracksFound.replace(
"{query}",
Expand All @@ -61,7 +60,7 @@ export const createInsertCommand: CreateCommandFunc = (

const queue = player.getQueue();

const track = firstResult.tracks[0];
const track = searchResult.tracks[0];
player.insert(track, trackIndex);

// human friendly track position
Expand Down
11 changes: 5 additions & 6 deletions src/engines/file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { StreamType } from "@discordjs/voice";
import { stat } from "fs/promises";
import { IAudioMetadata, parseFile } from "music-metadata";
import path from "path";
import { PlayerEngine, SearchResult, Track } from "../types/engines";
import { PlayerEngine, Track } from "../types/engines";
import { isSubPath } from "../utils/fs";

async function isFile(path: string): Promise<boolean> {
Expand All @@ -24,15 +24,15 @@ export const fileEngine = {
},
async search(query, playerOptions) {
// refuse to search files outside of fileRoot
if (!this.isResponsible(query, playerOptions)) return [];
if (!(await isFile(query))) return [];
if (!this.isResponsible(query, playerOptions)) return null;
if (!(await isFile(query))) return null;

// get file metadata
let metadata: IAudioMetadata;
try {
metadata = await parseFile(query, { skipCovers: true });
} catch (e) {
return [];
return null;
}

const track: Track = {
Expand All @@ -43,8 +43,7 @@ export const fileEngine = {
source: this.source,
};

const searchResult: SearchResult = { tracks: [track], source: this.source };
return [searchResult];
return { tracks: [track], source: this.source };
},
async getStream(track, playerOptions) {
// refuse to stream files outside of fileRoot
Expand Down
Loading

0 comments on commit 503ab0b

Please sign in to comment.