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

Refactor use-search to use a properly-typed fuzzy-search library #197

Closed
2 tasks
Tracked by #110 ...
mazipan opened this issue Jul 20, 2021 · 7 comments
Closed
2 tasks
Tracked by #110 ...

Refactor use-search to use a properly-typed fuzzy-search library #197

mazipan opened this issue Jul 20, 2021 · 7 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed ui User Interface ux User Experience

Comments

@mazipan
Copy link
Member

mazipan commented Jul 20, 2021

Brief

The current useSearch() implementation uses the itemsjs library, which has no proper typings. This leads to useSearch() being littered with any type annotations.

We want to avoid using any if possible. Therefore, we should migrate to a search library with proper typings (e.g. fusejs).

Definition of done

  • use-search properly migrated to a library with valid typings.
  • All any are replaced by correct typings.

Tasks

  • Migrate to a properly typed fuzzy search library
  • Refactor use-search to use fusejs and add proper typings
@mazipan mazipan added the enhancement New feature or request label Jul 20, 2021
@resir014
Copy link
Member

useSearch() also uses the itemsjs library, which has no proper typings.

Highly recommend switching to a properly typed library, e.g. fusejs

@resir014 resir014 changed the title typings - any on use-search Refactor use-search to use a properly-typed fuzzy-search library (fusejs) Jul 20, 2021
@resir014
Copy link
Member

Issue + tasks list updated.

@resir014 resir014 changed the title Refactor use-search to use a properly-typed fuzzy-search library (fusejs) Refactor use-search to use a properly-typed fuzzy-search library Jul 20, 2021
@mazipan
Copy link
Member Author

mazipan commented Jul 20, 2021

Itemsjs

https://bundlephobia.com/package/[email protected]

Size: 64kB
Minified: 20kB

Fuse.js

https://bundlephobia.com/package/[email protected]

Size: 15kB
Minified: 5kB

But yes, this is a big refactor.
Since you will face many differences between this two libraries.

@mazipan mazipan mentioned this issue Jul 20, 2021
2 tasks
@mazipan
Copy link
Member Author

mazipan commented Jul 21, 2021

Since this issue is classified as hard-level. I think it's better if we introduce an automated testing first for this component before go on the refactoring part.

The hard part is you need to tackle all cases, testing it, make sure it's not breaking. From my personal experience, testing using a manual way when you're not the original creator is very risky.

@resir014 @zainfathoni

@mazipan mazipan added ui User Interface ux User Experience labels Jul 21, 2021
@resir014
Copy link
Member

resir014 commented Jul 26, 2021

Found more issues with itemsjs: turns out it's using a vulnerable version of lodash:

image

A quick peek through the bundle analyser shows that it's coming from itemsjs. We missed out on this initially, because turns out the dependency is inlined within itemsjs itself:

image

We need to expedite moving away from itemsjs. Should we proritise this issue, or should we wait until the Typesense experiment at #374 is complete? Or more importantly, which one of these takes the least effort?

cc: @zainfathoni @mazipan @ekamuktia @famasya

@famasya
Copy link

famasya commented Jul 26, 2021

As my experience, integrating Typesense into React component itself isn't take too much effort (and properly typed). I think we should take a look at #401 first, since it's having some minor issues @resir014 .

cc @zainfathoni @mazipan @ekamuktia

@baraeb92 baraeb92 added the help wanted Extra attention is needed label Aug 4, 2021
@baraeb92 baraeb92 added this to the Long-term Plan milestone Aug 4, 2021
@zainfathoni
Copy link
Member

Closing this issue in favour of the #374.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed ui User Interface ux User Experience
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants