jeancroy / FuzzySearch

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

Fix confusion around how public `add()` method works #10

Closed aguynamedben closed 6 years ago

aguynamedben commented 6 years ago

Rename _prepSource, change add() method signature

Discussion on why these changes were made can be found here: https://github.com/jeancroy/FuzzySearch/issues/9

In summary, the public add() method had some ambiguity on how it behaved which sometimes led to unexpected results (i.e. item missing in search results after you add it).

jeancroy commented 6 years ago

How do you plan to use source together with add ?

I ask because the use case in my head is that server will suggest entries but slower than our instant search. Unless the server keep an history, it's very likely to send duplicate. Do we care about avoiding duplicate ? If so, an unconditional this.source.push is not going to suffice.


Also I keep babbling about a private _add. The reason I keep babbling about a private add is this

 _buildIndexFromSource: function () {
      // ...
     for (var item_index = -1; ++item_index < nb_items;) {
                var source_item = this.source[item_index];
                this.add(source_item);
    }
}

// ...

add: function(source_item){
   //...
   this.source.push(source_item);
}

There's some kind of "circular reference" if we use the same add internally. As you currently have setup things, source will double length each time we recompute index, I think.


Add tests! Uses Mocha, Chai,

Yay !

and JSDom (because access to window is presumed, but this could go away eventually)

No ... I first test for the existence of window and window performance timer. - Only if both exist I use them, else simply date.now(). I never presume window exist, I check for it, does it make sens ?. If people keep tripping over that detail, I may remove it.

I use it to measure how long a search take & adjust debounce time. I used performance counter because the thing we are trying to measure is similar of the incertitude of date.now(). I also did not used nodeJS high resolution time, because I do not think server has to debounce key presses.

jeancroy commented 6 years ago

Maybe the most simple thing is to rename current add index

then


add: function(source_item)(){
     this.source.push(source_item)
     this.index_item(source_item)
},

index_item: function(source_item){
  // current add
},

_buildIndexFromSource: function () {
    // ...
   // foreach item
       this.index_item(source_item)
}

that way if anyone has a special need I'll direct them to build their own add function using index.

Maybe I'll also remove the whole "lazy" index building idea. That way making a search is never destructive. (though I do realize it was a false flag for your previous problem)

jeancroy commented 6 years ago

So thank you for your work on this.

This is what I ended up doing, please tell me if it seems ok to your need. It does pass your test suite.

searcher.add(source_item, should_update_source)
aguynamedben commented 6 years ago

Hey @jeancroy, sorry for the delay on this. That all sounds great on first read. I'll take a close look at the merged code tonight and let you know if I have any more advanced opinions!

Thanks so much for the help and being an A+++ repo maintainer. Feel free to send small issues my way if you want help hacking on this repo.

aguynamedben commented 6 years ago

@jeancroy Just read your reasoning and the code you added after merging and I really like it. Great simple API. Thanks! 👏