nextapps-de / flexsearch

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

If you have problems with the default typings on 0.7.31 or with the `index.export` method, check this #406

Closed iivvaannxx closed 10 months ago

iivvaannxx commented 1 year ago

It seems that this repository is no longer maintained. It didn’t receive any commits in the whole year except for one, on July which only modified the README to include some donation links.

The latest version left the package in an unusable state with Typescript, because it ships with a set of wrong types. The correct ones are located at DefinitelyTyped, so you need to install @types/flexsearch. The problem is that they are overriden by the default typings. At the end of this issue you will find a patch to remove these default types.

Also, there's the index.export function which is typed as an async function because it’s supposed to be async, but it’s actually not. It runs “asynchronously” but it doesn’t return a Promise, so you can’t await the method to finish. See: #353 and #393.

With the PR #374 (thanks! @thexeos), I created a patch file using pnpm patch after rebuilding the package from source using the fix. In my case it is installed by copying it to a patches folder at the root directory of my project and modifying the package.json like this:

"pnpm": {
  "patchedDependencies": {
    "flexsearch@0.7.31": "patches/flexsearch@0.7.31.patch"
  }
}

But it should also work with patch-package. You can find the patch here. After installing it you should be able to await the index.export method. If you are only interested in removing the typings you can use only this specific part from the patch:

diff --git a/index.d.ts b/index.d.ts
deleted file mode 100644
index 9f39f41073864b83968bdaa242ac4e3c3149685a..0000000000000000000000000000000000000000
diff --git a/package.json b/package.json
index 8968f5bf8010ff194240591c8b83299f7328e79d..08e41ce167152f647d6e344bd2fd85787abfbba8 100644
--- a/package.json
+++ b/package.json
@@ -23,7 +23,6 @@
   "main": "dist/flexsearch.bundle.js",
   "browser": "dist/flexsearch.bundle.js",
   "module": "dist/module/index.js",
-  "types": "./index.d.ts",
   "preferGlobal": false,
   "repository": {
     "type": "git",
@@ -50,7 +49,6 @@
     "dist/**",
     "src/**",
     "task/**",
-    "index.d.ts",
     "README.md",
     "CHANGELOG.md",
     "LICENSE"

I hope this can be useful, it solved my problems with both bugs in a fresh project I tested it on.

Properko commented 1 year ago

Thanks for the tips. Reg. types in typescript it's also possible to overwrite the emitted declarations.

If ./src/types is in your typeRoots in tsconfig.json, you can create ./src/types/flexsearch/index.d.ts to override the included types with DefinitelyTyped:

import FlexsearchTypes from "@types/flexsearch/index.js";
declare module "flexsearch" {
  export = FlexsearchTypes;
}

That said there are also other breaking changes in the recent versions, so just doing this won't save you from the headache. E.g. for the "async" export I did

/**
 * @param indexDoc flexsearch index document
 * @param filePath path to export the index to
 */
const exportAsync = (
  indexDoc: flexsearch.Document<{ title: string; content: string; href: string; tag: string[] }, string>,
  filePath: string
) =>
  new Promise<void>((resolve, reject) => {
    try {
      logger.info(`Exporting document index files to '${filePath}'`);
      return indexDoc.export(async (key, data: any) => {
        try {
          await fsPromises.mkdir(filePath, { recursive: true });
          const filename = `${filePath}/${key}.json`;
          await fsPromises.writeFile(filename, data || "");
          logger.debug(`Exported '${key}' document index`);
          // exported keys: [reg, title.cfg, title.map, title.ctx, content.cfg, content.map, content.ctx, tag, content.store]
          // so we will resolve on store
          // it's ugly to rely on the internals like this
          // flexsearch needs to be updated to indicate the export is finished
          // https://github.com/nextapps-de/flexsearch/issues/290
          if (key === "content.store") {
            resolve();
          }
        } catch (err) {
          reject(err);
        }
      });
    } catch (err) {
      reject(err);
    }
  });

but ultimately, I stuck to 0.7.21 and will probably patch as well.

iivvaannxx commented 1 year ago

Thanks! I didn't know you could override types like that. If the export method is not working for you even with the patch, the problem may be you are trying to export from a Document object. I am not sure they use the same API internally, so the export patch may not apply to this objects. I didn't test it, so I don't know.

After creating the patch, I run into some other problem which I don't remember now with 0.7.31 (as you said, this doesn't save from all the headaches) and ended up rolling back again to 0.7.21. I also created a patch for that version so I could use the export method as expected (as I said, this is for Index instances and it may not work with Document objects).

In case anyone needs it, you can find it here. Version 0.7.21 does not come with default breaking types, so this patch doesn't need to handle that, it only fixes the index.export method.

Pilaton commented 1 year ago

This package is so crooked that you're better off using another one. There are plenty of them.

pacoorozco commented 1 year ago

@Pilaton can you point out some alternatives? I've been looking around and I've not found one that can replace the way I'm using flexsearch. Basically, it is able to index Markdown documents (including the content) and search inside the documents to give sections as a result.

Thanks in advance.

ts-thomas commented 10 months ago

@iivvaannxx I recently merged some PRs, can you check please if something is missing?

iivvaannxx commented 10 months ago

@ts-thomas I've done a couple of tests on a new project. At the moment I can confirm that the type declaration files are not picked out of the box, after installing flexsearch Typescript complained and I fixed it by installing the @types/flexsearch module.

The module is still a CommonJS module which prevents you from importing anything via import like the following:

import { Index } from 'flexsearch'

If you try it Vite will complain with the following error:

SyntaxError: [vite] Named export 'Index' not found. The requested module 'flexsearch' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'flexsearch';
const {Index} = pkg;

And if you import it as the error says it works. But, the export method is still not working for me as one would expect. I have the following code:

import FlexSearch from "flexsearch"
import type { PageServerLoad } from "./$types"

export const load: PageServerLoad = async ({ fetch }) => {

  const response = await fetch('src/options.json')
  const fileContent = await response.json() as any[]

  const { Index } = FlexSearch
  const options = new Index({

    preset: 'performance',
    tokenize: 'reverse',
    cache: 200
  })

  fileContent.forEach((option, index) => {

    options.add(index, option.name)
  })

  const dataIndex = { }
  await options.export((key, value) => {

    console.log("Exporting", key)
    Object.assign(dataIndex, { [key]: value })
  })

  console.log("Export Done!")
  return dataIndex
}

This is a simple snippet I made on a brand new SvelteKit project just to test the package as I was asked. The file options.json is quite a big JSON file (~2MB). When you try to export the data, the await keyword should make the code wait for the export before logging "Export Done", but instead what I get is this:

Export Done!
Exporting reg
Exporting cfg
Exporting map
Exporting ctx
thexeos commented 10 months ago

Hmm. The library does not return a Promise for Index exports, only for Document. I can see how that can be confusing. I will see if there is a way to return a working Promise for Index exports without adding extra overhead.

Disregard that. A Promise should be returned at all times when calling exportIndex or exportDocument directly.

@ts-thomas, I'm not seeing the new code under dist in NPM.

ts-thomas commented 10 months ago

@iivvaannxx we need to have both, ESM support and Node.js support, so thats why they exist 2 different builds. That is what I have actually in package.json

  "main": "dist/flexsearch.bundle.js",
  "browser": "dist/flexsearch.bundle.js",
  "module": "dist/flexsearch.module.bundle.js",

Node.js will pick "main" by default, so here it is a CommonJS export, used by Node.js as default. Bundler will pick the "browser" by default, here it gets UMD including CommonJS. I don't know if "module" will recognized. The question is, what should be the default export for "browser"?

https://github.com/defunctzombie/package-browser-field-spec

ts-thomas commented 10 months ago

@thexeos I will make a new build later today. Actually I'm updating the whole build stuff.

ts-thomas commented 10 months ago

Also please take a look here: https://github.com/nextapps-de/flexsearch/discussions/417

It isn't online yet, but is coming later today. There are 2 versions of ESM modules:

  1. one is bundled (all methods are exported as static on FlexSearch.xxx)
  2. the other one is non-bundled (methods will export as plain)

Which of both is better for the "browser" field in the package.json and which one is better for "main"? The "index.d.ts" isn't compatible with both, do I need to decide to have a typescript definition just for one of them, or can I have a definition for each of them?

thexeos commented 10 months ago

If you can make the bundled one be exposed as a separate module of the package (see below), then you can default to granular (non-bundled) export with appropriate types.

{
  "type": "module",
  "exports": {
    ".": {
        "main": "dist/flexsearch.non-bundle.js",
        "browser": "dist/flexsearch.non-bundle.js",
        "module": "dist/flexsearch.module.non-bundle.js",
        "types": "dist/flexsearch.non-bundle.d.ts"
    },
    "bundle": {
        "main": "dist/flexsearch.bundle.js",
        "browser": "dist/flexsearch.bundle.js",
        "module": "dist/flexsearch.module.bundle.js",
        "types": "dist/flexsearch.bundle.d.ts"
    }
  }
}

Although, I'm not sure this is the most elegant solution, as it leads to a different question: why is bundled even included in the "exports"? If bundled file is only ever used via direct reference in the browser (no bundlers involved), then it can be linked directly via full path (<script src="node_modules/flexsearch/dist/flexsearch.bundle.min.js"></script>). And if bundler is involved, then then [the bundler] will prefer to see individual files.

Node.js resolver would be happy to navigate the file-system and load individual (non-bundled) files. The penalty is only incurred at startup and with the size of an average node_modules folder these days, extra 20 files from flexsearch won't make the weather.