[ps-136] Moved normalizeSearchQuery method to Utils

bitwarden / jslib

Common code referenced across Bitwarden JavaScript projects.

https://bitwarden.com

GNU General Public License v3.0

134 stars 138 forks source link

[ps-136] Moved normalizeSearchQuery method to Utils #817

Closed gbubemismith closed 2 years ago

gbubemismith commented 2 years ago

Type of change

[X] Bug fix
[ ] New feature development
[ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
[ ] Build/deploy pipeline (DevOps)
[ ] Other

Objective

Code changes

common/src/services/search.service.ts: referenced utils class and changed normalizeSearchQuery call to use the utils method.
common/src/misc/utils.ts: moved normalizeSearchQuery here so it can be accessed by lunr pipeline function.

Before you submit

[X] I have checked for linting errors (npm run lint) (required)
[ ] I have added unit tests where it makes sense to do so (encouraged but not required)
[ ] This change requires a documentation update (notify the documentation team)
[ ] This change has particular deployment requirements (notify the DevOps team)

Hinton commented 2 years ago

Wasn't this already merged in #812?

gbubemismith commented 2 years ago

Wasn't this already merged in #812?

Yeah it was but I discovered a bug when I moved the normalisation to a private method. The lunr pipeline function can't access methods outside it's scope.

Hinton commented 2 years ago

We could just a private internal function in the file and use it?

gbubemismith commented 2 years ago

do you mean like a private normalizeQuery function and then call the function with this.normalizeQuery in the pipeline function???

Hinton commented 2 years ago

Yes, just moving normalizeQuery to be a top level function. That should keep it accessible in the lunr pipeline (or so I assume).

gbubemismith commented 2 years ago

Yes, just moving normalizeQuery to be a top level function. That should keep it accessible in the lunr pipeline (or so I assume).

Moved it to the utils class

gbubemismith commented 2 years ago

Utils is a bit of a miscellaneous class, we shouldn't add to it if we can avoid it.

I would recommend just making normalizeSearchQuery a public method on SearchService, will that work for lunr? Or if it needs to be static, make it static on SearchService. You can do either without moving it to Utils.

@Hinton I don't quite understand your feedback, is this what you were suggesting?

made it a static method in SearchService

Hinton commented 2 years ago

@eliykat I'm fine with static on SearchService. My initial thought was to create a function outside the class in the same file. Javascript isn't locked to OOO and we can diverge if needed.