nextapps-de / flexsearch

Next-Generation full text search library for Browser and Node.js
Apache License 2.0
12.4k stars 490 forks source link

Incorrect type definitions #342

Open dixiekong opened 2 years ago

dixiekong commented 2 years ago

The supplied type definitions are not correct importing the Index as such:

import { Index } from 'flexsearch';
const index = new Index();

results in the error: 'Index' only refers to a type, but is being used as a value here

There exits working type definitions in https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/flexsearch but to use them one has to either downgrade to a version containing no type definitions (e.g. 0.7.11) or delete the supplied one.

It would be awesome if you can sort this out.

darrenmothersele commented 2 years ago

I'm using patch-package so that I can continue to use @types/flexsearch by deleting the provided index.d.ts file.

BasLee commented 2 years ago

Same problem here when I try to create a Document index with version 0.7.31. When importing like import {Document} from 'flexsearch' the provided typings point to:

  interface Document {
    id: string;
    field: any;
  }

But it should point to something like this

klimashkin commented 1 year ago

After updating from 0.7.21 to 0.7.31 it's completely unclear how to import Document and Index. Is there a guide or changelog to go through?

For now that feels like a breaking change

frankleng commented 1 year ago

After updating from 0.7.21 to 0.7.31 it's completely unclear how to import Document and Index. Is there a guide or changelog to go through?

For now that feels like a breaking change

had to fork it - https://www.npmjs.com/package/flexsearch-ts and swap types with @types/flexsearch happy to PR if others find this useful...

lucas-labs commented 1 year ago

I'm experiencing this issue after updating from v0.7.21 to v0.7.31 (I was using this lib with @types/... definitions before). As of right now it seems completely unusable with typescript (I mean, it works once compiled but it throws type errors everywhere on dev time).

I had to rollback to 0.7.21 for the time being.

chuanqisun commented 1 year ago

Same issue after upgrading to 0.7.31. The DefinitelyTyped version (which is arguably the correct typing) is being shadowed by the built-in typing at node_modules/flexsearch/index.d.ts. I have to downgrade to 0.7.21.

In the meanwhile, @ts-thomas (author of Flexsearch) and @Losses (Author of de-facto the best typing in DefinitelyTyped project), would you consider bringing the DefinitelyTyped version into this repo? The typing files won't hurt performance and it's in theory a O(1) complexity change with huge improvement for developers who expect out-of-box typing. The competing library Lyra search, for example, already ships with full typescript support.

chuanqisun commented 1 year ago

I also saw #343 trying to overhaul the typing. There is probably conflict change. Would it help or make it worse if we break that into a separate typing update plus other bigger changes for later, just to unblock existing typescript users? Just throwing some ideas out there.

ts-thomas commented 1 year ago

Thanks for this report. The whole ts-type dilemma is pretty strange. This is the only part of code which wasn't contributed by me. I really would like to have some advice, which of the type definitions are the proper one and what needs to be changed. If someone could creating a pull request, that would be great.

Thanks a lot.

Losses commented 1 year ago

Same issue after upgrading to 0.7.31. The DefinitelyTyped version (which is arguably the correct typing) is being shadowed by the built-in typing at node_modules/flexsearch/index.d.ts. I have to downgrade to 0.7.21.

In the meanwhile, @ts-thomas (author of Flexsearch) and @Losses (Author of de-facto the best typing in DefinitelyTyped project), would you consider bringing the DefinitelyTyped version into this repo? The typing files won't hurt performance and it's in theory a O(1) complexity change with huge improvement for developers who expect out-of-box typing. The competing library Lyra search, for example, already ships with full typescript support.

I'd like to if the developers of this library agree to merge the pull request... https://github.com/nextapps-de/flexsearch/pull/266

nickreese commented 1 year ago

For all of those struggling with this using pnpm here is how to install the @types/flexsearch and overwrite the package types.

  1. pnpm add @types/flexsearch
  2. pnpm patch flexsearch
  3. open the folder given by patch... remove index.d.ts
  4. pnpm patch-commit [folder given in step 2]
itPiligrim commented 1 year ago

Up

rap2hpoutre commented 1 year ago

Up.

yenche123 commented 1 year ago

For all of those struggling with this using pnpm here is how to install the @types/flexsearch and overwrite the package types.

  1. pnpm add @types/flexsearch
  2. pnpm patch flexsearch
  3. open the folder given by patch... remove index.d.ts
  4. pnpm patch-commit [folder given in step 2]
  1. Restart VS-Code, and then it will work it out
regnete commented 1 year ago

this fixes he issue for me (workarround only)

npm i -D flexsearch
npm i -D @types/flexsearch
rm -rf node_modules/flexsearch/index.d.ts
fgarcia commented 1 year ago

Just came here wondering if the library was dropped because of how broken the type definitions are. I believe it would be much better to ship without types (just delete index.d.ts) for the sake of not sending wrong signals about the status of the project.

NuroDev commented 1 year ago

100% agree with @fgarcia here. If the official type definitions are not going to be actively maintained but the library is then at least remove them so @types/flexsearch can be managed by the community.

OneCyrus commented 1 year ago

@ts-thomas would be great if you could remove the index.d.ts from the package/release

basically merging this PR: https://github.com/nextapps-de/flexsearch/pull/366

Losses commented 1 year ago

@ts-thomas would be great if you could remove the index.d.ts from the package/release

basically merging this PR: #366

As the author of @types/flexsearch, my heart breaks every time when someone replies to this issue and I received the notification. Feel my despair sir. (ㆆᴗㆆ)

robigan commented 1 year ago

Bump.

Although, if I were in the shoes of @ts-thomas I would drop index.d.t.s and let the community manage the TS typings. As he has said in this thread (quoted below) he isn't sure about what should be done as the community has varying opinions on how the library should go forward in this regard, so I'd say his best bet is to drop index.d.ts

Thanks for this report. The whole ts-type dilemma is pretty strange. This is the only part of code which wasn't contributed by me. I really would like to have some advice, which of the type definitions are the proper one and what needs to be changed. If someone could creating a pull request, that would be great.

Thanks a lot.

romanfq commented 1 year ago

After updating from 0.7.21 to 0.7.31 it's completely unclear how to import Document and Index. Is there a guide or changelog to go through? For now that feels like a breaking change

had to fork it - https://www.npmjs.com/package/flexsearch-ts and swap types with @types/flexsearch happy to PR if others find this useful...

I am trying to use your fork and still doesn't work for me. Error below

$ npm run index

> flexsearchfail@1.0.0 index
> tsc --outDir dist && cd dist && node index.js

file:///Users/mememe/Documents/Football/FlexSearchFail/dist/index.js:27
    var doc = new fst.Document({
              ^

TypeError: fst.Document is not a constructor

The code for index.ts looks like this

import * as fst from 'flexsearch-ts';
import { Player } from './player.js';

(async () =>{
    const players = [
        new Player("928fd47b", "Zlatan Ibrahimovic", "03-10-81"),
        new Player("dd76a750", "Lionel Messi", "24-06-87"),
        new Player("6177f012", "Cristiano Ronaldo", "05-02-85"),
    ];
    var doc = new fst.Document<Player, false>({
        document: {
            id: "identifier",
            index: ["name", "dob"]
        }
    });
    players.forEach(p => doc.add(p.id, p));

    doc.searchAsync("Messi").then(searchResults => {
        searchResults.forEach(sr => {
          console.log(sr.result);
        })
    });
})();

package.json:

{
  "name": "flexsearchfail",
  "type": "module",
  "version": "1.0.0",
  "description": "Repro steps to fail at using FlexSearch from a typescript node app",
  "main": "index.ts",
  "scripts": {
    "build": "tsc --outDir dist",
    "index": "tsc --outDir dist && cd dist && node index.js"
  },
  "author": "me",
  "license": "MIT",
  "devDependencies": {
    "ts-node": "^8.2.0",
    "tslint": "^5.17.0",
    "typescript": "^4.9.5"
  },
  "dependencies": {
    "flexsearch-ts": "^0.7.35"
  }
}