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(index): add search function #181

Merged
merged 8 commits into from
May 8, 2022
Merged

Conversation

kyrie25
Copy link
Member

@kyrie25 kyrie25 commented May 2, 2022

  • Adds search function for Marketplace (or at least finishes it), thought this was much needed
    Currently compare search query with name/user and filters array
    Spotify_cDEYQ9TSfY

  • Line 843: Unnecessary condition ig?
    image

Copy link
Member

@theRealPadster theRealPadster left a comment

Choose a reason for hiding this comment

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

Hey thanks for tackling this! Looks good; I just had one suggestion.

My other concern is the fact that we're using paging and caching now. I was thinking the search filter would send an actual search query to github and fetch results frrom there. If we just filter what's in the cards variable, it could miss things if they haven't all been loaded. Not sure if it makes sense to add in the meantime (since it could lead to bug reports about the search missing things). Thoughts @CharlieS1103 ?

index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
@kyrie25
Copy link
Member Author

kyrie25 commented May 2, 2022

If we just filter what's in the cards variable, it could miss things if they haven't all been loaded.

I'm not sure it would be that much of an issue since all the GitHub results are loaded almost instantly every time Spotify starts (at least for me) but yeah that does make sense
image

@kyrie25
Copy link
Member Author

kyrie25 commented May 2, 2022

Also I've found out that sending requests continuously to GitHub's Search API can eventually lead to a bottleneck as onChange is fired on every new input so ig we'll have to implement an event that sends a request after onChange has been fired for a certain period of time? (or that it only fires upon pressing Enter)

Timeouts and incomplete results
To keep the Search API fast for everyone, we limit how long any individual query can run. For queries that exceed the time limit, the API returns the matches that were already found prior to the timeout, and the response has the incomplete_results property set to true.
Reaching a timeout does not necessarily mean that search results are incomplete. More results might have been found, but also might not.

@theRealPadster
Copy link
Member

theRealPadster commented May 2, 2022

Also I've found out that sending requests continuously to GitHub's Search API can eventually lead to a bottleneck

Yes, we would have to throttle it or something. Or use a hybrid where we search the cards like you're currently doing sometimes maybe 🤔

Copy link
Member

@CharlieS1103 CharlieS1103 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Seems all good to me. One functionality we may want to add in the future is searching by tag.

@CharlieS1103
Copy link
Member

Just took a second look and seems like we may run into issues using github requests, might need to migrate the onchange to use the caching system i implemented

@kyrie25
Copy link
Member Author

kyrie25 commented May 2, 2022

Or use a hybrid where we search the cards like you're currently doing sometimes maybe 🤔

I'm pretty sure if we were to use GitHub API for search queries it wouldn't be much of a problem to implement a hybrid system as this is possible

if (allThemes.incomplete_results === true) {
        // Filter results from cache
    }

I'm not sure about how to deal with the throttle part tho, gonna have to look into it a bit more.

@kyrie25
Copy link
Member Author

kyrie25 commented May 2, 2022

I've found out also that if you were to send an incomplete keyword (such as play instead of playlist) the API would return fewer results than the actual keyword (even though it still includes playlist results), and that it would be difficult to differentiate between a repository/author/tag/etc. query, apart from having users write user: themselves, so I'm not sure there's any possible way to implement live GitHub API results and maintaining a good UX at the moment
May I have your opinions/ideas on this? @theRealPadster
image
image

@kyrie25
Copy link
Member Author

kyrie25 commented May 2, 2022

Just took a second look and seems like we may run into issues using github requests, might need to migrate the onchange to use the caching system i implemented

Oh I have yet to implement GitHub API calls for search queries so I don't think that's a problem yet

@theRealPadster
Copy link
Member

I've found out also that if you were to send an incomplete keyword (such as play instead of playlist) the API would return fewer results than the actual keyword (even though it still includes playlist results)

Woah, that is bizarre.
I did this search of play vs playist. It looks to me like the ones that don't show up for "play" but do for "playlist" have the word "playlist" in text fields or tags, and nowhere in any URLs. Maybe it tries to match full words but just accepts full url strings?

I think the best solution for rate limits etc would be to actually just not have live search and to have it load a search page when you hit enter. That solves the query issue, and people will type the entire thing in.

We could also do your card filtering method for live results, and then have a full search page if they press enter. Thoughts?

@kyrie25
Copy link
Member Author

kyrie25 commented May 2, 2022

I think the best solution for rate limits etc would be to actually just not have live search and to have it load a search page when you hit enter. That solves the query issue, and people will type the entire thing in.

We could also do your card filtering method for live results, and then have a full search page if they press enter. Thoughts?

That is a pretty good idea, I agree. I was thinking about caching the initial results also, and if GitHub returns 0 results we could filter out from the cache and give the results from there.

Oh and please feel free to commit directly to the PR once you find a working solution :) . I might not be able to find one in a timely manner (mainly because this is the first time I've worked with React) so I'd appreciate any help also.

@kyrie25
Copy link
Member Author

kyrie25 commented May 3, 2022

Now filters existing results upon input, fetch live GitHub API results (aka. refresh results) upon pressing Enter key
@theRealPadster This should be satisfactory yes?
Also refactored loadPage() using switch... case... instead of if... else for perfomance

Spotify_iLN0jGxYKJ

@kyrie25
Copy link
Member Author

kyrie25 commented May 3, 2022

Addresses some small safety checks
image

@theRealPadster
Copy link
Member

Hey sorry, I've been super busy lately. Really do appreciate the contributions though. I'll look as soon as I can. Wanna get the code running locally.

@kyrie25
Copy link
Member Author

kyrie25 commented May 5, 2022

Now uses loading icon when fetching queries
Spotify_bizwOWy8SX

Copy link
Member

@theRealPadster theRealPadster left a comment

Choose a reason for hiding this comment

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

Loading icon is sweet! I have a couple suggestions. I'd also like it mention that we are (I am) in the process of converting the app to be built with Spicetify-Creator, which lets us use jsx and typescript and other niceties. So these changes will need to be replicated once that is ready. Do you think you'd be able to get the functionality moved over once we've moved?

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
@kyrie25
Copy link
Member Author

kyrie25 commented May 5, 2022

Loading icon is sweet! I have a couple suggestions. I'd also like it mention that we are (I am) in the process of converting the app to be built with Spicetify-Creator, which lets us use jsx and typescript and other niceties. So these changes will need to be replicated once that is ready. Do you think you'd be able to get the functionality moved over once we've moved?

Sure, I'll look into it! Do give me a mention on GitHub or Discord once you need it going.

@CharlieS1103
Copy link
Member

Going to go ahead and just merge this in, we'll get it moved over to JSX when the time comes.

@CharlieS1103 CharlieS1103 merged commit 75f5da1 into spicetify:main May 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants