krisk / Fuse

Lightweight fuzzy-search, in JavaScript
https://fusejs.io/
Apache License 2.0
18.1k stars 766 forks source link

FuseOptions<T> prevents nested key search #261

Closed LiamLB closed 5 years ago

LiamLB commented 5 years ago

Version: 3.3.0 Type: Potential Bug

Hi,

With the recent addition of typing to FuseOptions and Fuse nested key search is now giving Typescript compile errors.

The particular culprit is this line in the FuseOptions interface

keys?: (keyof T)[] | { name: keyof T; weight: number }[];

An example of how this was used in my application:

  fuseOptions: FuseOptions<Template> = {
    shouldSort: true,
    threshold: 0.6,
    keys: [
      { name: 'metadata.title', weight: 0.8 },
      { name: 'metadata.category', weight: 0.1 },
      { name: 'metadata.summary', weight: 0.1 },
    ],
  };

Where metadata was a key in template and title is a key in metadata. This is no longer allowed by the interface. As a temporary solution I've included a // @ts-ignore above the fuseOptions definition and functionality is fully #restored.

krisk commented 5 years ago

@ceymard want to take a look at this?

ceymard commented 5 years ago

Unfortunately such a thing is not possible. Typescript has no idea of how to handle path specifications since they're completely implementation specific.

My best guess is that we should then go back to string instead of keyof T and forego the <T> in the type.

The sad thing about that is that you lose the robustness that comes with checking that the name indeed does exist.

I don't know if the library supports it, but a good solution imho would be able to specify extractor functions. Not only would that probably be faster than parsing a path and resolving it, this would allow for some pretty strongly checked typing in typescript.

The implications is that a key could then be either specified by a string or a function.

This is the way that this could work ;


// An extractor is either a key for the object or an extracting function
// Here, I supposed the algorithm handles null or undefined
// values -- I suppose it does since path specifying can end up with non-existant properties
type Extractor<T> = keyof T | ((obj: T) => string | null | undefined)

// FuseOptions would become
interface FuseOptions<T> {
  // ... all the rest
  keys: Extractor<T>[] | { name: Extractor<T>; weight: number }[]
}

// By the way, I'm not clear on if we can mix simple keys with weighted ones.
// In this case, keys would be :
// (Extractor<T> | { name: Extractor<T>; weight: number })[]

  fuseOptions: FuseOptions<Template> = {
    shouldSort: true,
    threshold: 0.6,
    keys: [
      // This way, all the properties are properly checked. We are certain that they exist,
      // and if one day the code changes for the metadata object, this code will then
      // become an error, as it should.
      { name: t => t.metadata.title, weight: 0.8 },
      { name: t => t.metadata.category, weight: 0.1 },
      { name: t => t.metadata.summary, weight: 0.1 },
    ],
  };

  // Even better, when using the fuse constructor, you don't have to type everything explicitely
  var my_list = /* ... */ // my list gets a type thanks to the compiler inference

  // All the following is checked since typescript infers T from my_list's type.
  var fuse = new Fuse(my_list, {
    shouldSort: true,
    threshold: 0.6,
    keys: [
      { name: t => t.metadata.title, weight: 0.8 },
      { name: t => t.metadata.category, weight: 0.1 },
      { name: t => t.metadata.summary, weight: 0.1 },
    ],
  });
ceymard commented 5 years ago

I could implement the changes in a pull request, if you're interested.

ceymard commented 5 years ago

To further explicit what I meant, the change in the code would look like this. I'm aware that there are more places to look for than just index.js@268, but that's just for the idea

          // this is the original extraction of the item
          key,
          value: this.options.getFn(item, key),

          // here is what I meant. This is untested and probably wouldn't work as is, but it's just a general idea
          key: typeof key === 'function' ? key.toString() : key,
          value: typeof key === 'function' ? key(item) : this.options.getFn(item, key)
ceymard commented 5 years ago

Or, even better (and sorry for the spam), respecify a getfn for each key.

  var fuse = new Fuse(my_list, {
    shouldSort: true,
    threshold: 0.6,
    keys: [
      { name: 'title', getfn: t => t.metadata.title, weight: 0.8 },
      { name: 'category', getfn: t => t.metadata.category, weight: 0.1 },
      { name: 'summary', getfn: t => t.metadata.summary, weight: 0.1 },
    ],
  });
favna commented 5 years ago

@ceymard Hi there, I just stumbled on this exact same issue and I'm glad it has been addressed already. Are you going to PR these changes into the main library so they can be included in a future stable release?

For now I'm fine with using yarn add ceymard/Fuse, however it would be nice to be working with stable versions.

doug-a-brunner commented 5 years ago

My workaround is to turn off type safety where I'm using nested properties, by adding a type assertion as { name: any, weight: number }[] to keys.

ceymard commented 5 years ago

@Favna I have no idea, I'm not a maintainer on this project - I used it for a while but I don't anymore.

I'm willing to do a PR, but I'll do so only if @krisk or another maintainer indicates that they're interested. After all, what I'm suggesting in the changes impacts the source code as well.

camposmichel commented 5 years ago

Hello guys I solved the problem by declaring my variable as any

const myItems = [...]

let fuseOptions: any = {
      keys: [
        "description"
      ]
};

const fuse = new Fuse(myItems, fuseOptions)
let response = fuse.search('water')

Hope this helps

favna commented 5 years ago

@camposmichel yeah that will indeed work. Personally I try to avoid any. The whole point of choosing to use TypeScript over vanilla JavaScript is to gain Type Safety and usingany` goes against that very principle as you're basically saying "this thing can be anything and I don't really care what it is".

Admittedly it certainly has it uses however so it's not like it should never be used.

camposmichel commented 5 years ago

@Favna I agree! But it was the way I found it to resolve quickly for me

favna commented 5 years ago

\ A better solution should be incoming if the here below referenced PR gets merged

favna commented 5 years ago

Updating the issue with the notice that this should (finally) be fixed if the here above referenced PR gets merged.

marianhlavac commented 5 years ago

Can someone explain how this is solved? The example code for TypeScript at https://fusejs.io is a complete nonsense. Tests aren't readable enough to be used as example for using nested keys and TypeScript together.

Would someone be so kind and add the missing examples of using nested keys (with weights preferably) in the TypeScript example on the website?

favna commented 5 years ago

@mmajko I can replicate the issue and it is something I overlooked when adding to the docs and improving the typings. That said, this is exclusively a typings thing so while I need time to look into the issue it should be noted that in the meantime you can still use it by adding // @ts-ignore above the keys line.

Confirmation of issue: image

favna commented 5 years ago

@mmajko #309 and #310 have been created to cover deep nested keys. Furthermore https://github.com/piotrwitek/utility-types/issues/85 has been created to provide an even better solution, although that won't be any near-future addition likely.