jeancroy / FuzzySearch

:mag: Fast autocomplete suggestion engine using approximate string matching
MIT License
194 stars 32 forks source link

Ability to index documents incrementally #5

Closed tehsven closed 7 years ago

tehsven commented 7 years ago

This patch provides the ability to add documents incrementally by using a new add method in conjunction with an identifyItem function. It does this by keeping track of the item ids in the indexmap dictionary where the values are the position into the index of where the item lives.

0001-Provide-the-ability-to-incrementally-add-to-the-inde.patch.zip

Additionally, this patch moves grunt into devDependencies instead of as a dependency.

0002-Make-grunt-a-devDependency.patch.zip

jeancroy commented 7 years ago

Ok so if I understand the patch


Out of those: I think the 3rd argument nb_indexed off add is a bit unfortunate. It prevent using the method easily and could be computed in the indexmap[itemId] === undefinedcase.

I think options.identifyItem could have a default value of null. In which case add is now append only. That would provide maximum backward compatibility because we did not request that items have an id field.

Alternatively keep a default of item[Id], and allow to set null to turn off merge behavior. Upon detection of basic source (list of string) default could be null.

tehsven commented 7 years ago

Thanks for taking a look at this @jeancroy!

Your assessment above is correct, except that add will never delete, it will only either append or replace.

I have addressed your two comments in the attached patch here. I have removed the nb_indexed parameter from the add method, since it is easily computed inside the add method. Also, identifyItem is null by default, which means that all calls to add will just be appended to the end of the index and no duplicate item detection will take place.

0001-Provide-the-ability-to-incrementally-add-to-the-inde.patch.zip

tehsven commented 7 years ago

Opened a PR for clarity: https://github.com/jeancroy/FuzzySearch/pull/6

jeancroy commented 7 years ago

Thank you for this contribution ! Is there other aspect you think this project could improve ?

tehsven commented 7 years ago

Thanks for getting this merged @jeancroy ! We are using this library in a few projects, and may have more suggestions in the future. For now, it is working very well for us.

mrkishi commented 7 years ago

This PR apparently has backwards incompatible changes. FuzzySearch stopped working on my project with an error upon search():

Uncaught TypeError: Cannot read property 'fields' of undefined

This is also happening on the demo.

jeancroy commented 7 years ago

Hi @mrkishi thanks for your report !

I think it is now fixed. We preallocated the array in _prepSource but then pushed to the end of the array in add. So the whole preallocated space is undefined, which cause error when reading in loop

I've added a counter for item that are actually in use.

mrkishi commented 7 years ago

That was way too fast, thanks! 💯

Took me days to notice it in the first place (since we don't usually update git-based dependencies).

In any case, it's on me for not pinning on a specific commit.