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

Search functionality should be centralized and implemented using established node packages #776

Open
jschanker opened this issue Oct 30, 2022 · 6 comments · May be fixed by #838
Open

Search functionality should be centralized and implemented using established node packages #776

jschanker opened this issue Oct 30, 2022 · 6 comments · May be fixed by #838
Assignees
Labels
code hygiene Non feature work, refactoring, infra enhancement New feature or request performance

Comments

@jschanker
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

  • There are a number of places in the codebase where search functionality is being implemented independently of each other (courses, filtering in tutor dashboard, etc.). In addition to unnecessarily duplicating code, it can lead to undesired non-uniform behavior. For example, a search for "some" might yield all results including this word in one place but only ones starting with "some" in another so "something" would appear in both search results but "awesome" would only appear in one.

  • Additionally, we will want to make search efficient as the list of items we're searching through becomes very large and minimize potential bugs.

Describe the solution you'd like
Create a search component that uses its algorithm for search from an established node package. One possibility would be to use trie-search. Once constructed, a trie will have O(m) search where m is the length of the search term (independent of the number of items being searched through) so this will provide fast search for prefixes. Keeping the data from https://github.com/joshjung/trie-search#search-or-of-multiple-phrases but completely changing the functionality to filter rows from the data Array for which ALL searched terms are prefixes of one of the words/numbers from name/age/zip:

const TrieSearch = require('trie-search');

const dataArr = [
  { name: 'andrew', age: 21, zip: 60600 },
  { name: 'andy', age: 37, zip: 60601 },
  { name: 'andrea', age: 25, zip: 60602 },
  { name: 'joseph', age: 67, zip: 60603 }
];

const keywordRowPairs = dataArr
  .map((dataRow) => ({ keywords: Object.values(dataRow).join(" "), dataRow }));
const trie = new TrieSearch("keywords");
trie.addAll(keywordRowPairs);
console.log(keywordRowPairs);
console.log(trie.search('60 and 25').map(({dataRow}) => dataRow)); // logs `{ name: 'andrea', age: 25, zip: 60602 }`

Describe alternatives you've considered
Investigate other packages with search functionality.

Additional context
This issue is prompted by the discussion with @iamvaiibhav19 about implementing filter functionality for the Tutor dashboard. The above solution can be adapted to implement this functionality.

@itsomkathe
Copy link
Contributor

@jschanker I searched for alternative packages, but this seems to be the latest. I will implement a class for searching based on all attributes.

@itsomkathe
Copy link
Contributor

@jschanker, take a look at this sandbox. I think this is the behavior that we want.

@jschanker
Copy link
Collaborator Author

@itsomkathe Looks good to me for basic search. In the future, we may want to add options such as limiting the number of results, only including results that start with the search word, sorting by the most relevant ones, etc.

Also, something we should really start doing more is documenting using JSDoc and including unit tests, so I'd recommend doing that for SearchService, if you have the time.

@itsomkathe
Copy link
Contributor

@jschanker I've added the documentation for the SearchService. For unit tests, I don't really have much idea about them.

@jschanker
Copy link
Collaborator Author

@itsomkathe All you would have to do is create a SearchService.test.js file and provide example inputs with expected outputs for search using https://jestjs.io/docs/expect. I wrote about this and unit tests for components in general here: #471 (comment). It's actually easier to do testing here since you're not testing UI. See also https://www.chaijs.com/api/ .

@itsomkathe
Copy link
Contributor

@jschanker seems like jest is also installed inside react-scripts, which is why it gave me an error while running the test when I installed jest again.

Capture2
Then I uninstalled the version which I had installed (latest)

Capture

@itsomkathe itsomkathe linked a pull request Dec 7, 2022 that will close this issue
@itsomkathe itsomkathe removed a link to a pull request Dec 9, 2022
@itsomkathe itsomkathe linked a pull request Dec 9, 2022 that will close this issue
@itsomkathe itsomkathe linked a pull request Dec 9, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code hygiene Non feature work, refactoring, infra enhancement New feature or request performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants