Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to stronger typings #229

Closed
D34THWINGS opened this issue Aug 29, 2023 · 2 comments
Closed

Switch to stronger typings #229

D34THWINGS opened this issue Aug 29, 2023 · 2 comments

Comments

@D34THWINGS
Copy link

I've been using minisearch for a few weeks now and I think the TypeScript typing is not really there. It's way too loosely typed with too many any being returned, this is quite dangerous and should not be done in a modern library.

The configuration options of the MiniSearch class should take into account the keys available given in the dynamic typing:

type Options<T = any> = {
    /**
     * Names of the document fields to be indexed.
     */
    fields: keyof T[];
}

Then the search result should also return only the storeFields if present or the entire record otherwise instead of Record<string, any> which is quite unsafe to use as you no longer have field validation on it.

If you agree with this I could open a pull request, but it would probably mean going for v7 of the package as this would be a breaking change for TS users.

@lucaong
Copy link
Owner

lucaong commented Aug 29, 2023

Hi @D34THWINGS ,
a similar issue was discussed in #208 . In short, the primary goal of MiniSearch is to support both JavaScript and TypeScript users with a simple and flexible API, that scales with user needs. Strict type safety in TS is a desirable feature, but subordinate to the goal above. Ultimately, I think the benefits of introducing such a breaking change do not outweigh the cost of.

Consider that, while by default documents are assumed to be plain key/value objects, MiniSearch does not force such assumption, and one can specify a custom extractField option to support any other pattern (see https://github.com/lucaong/minisearch#field-extraction). While there would be ways to support such feature in a type safe way, they would be a large breaking change, and make the API more clumsy and complicated for JS users. I like TypeScript, but MiniSearch should not assume that users are adopting it. The large majority of MiniSearch users come in fact from plain JS.

The current API is designed to be “additive”: common cases are simple, and more advanced options are layered on top, without making the basic API more complex.

I understand the appeal of a strict type design, but that tends to impose a level of complexity (even in the type definitions) that most users are not comfortable with. It's ultimately a matter of project direction, but ergonomics matter more than strict type safety for me in this case.

One viable option that does not involve breaking changes is to implement a strictly typed thin wrapper around MiniSearch.

@lucaong
Copy link
Owner

lucaong commented Aug 31, 2023

I think that the suggestion made by @juiceo here is quite interesting, and a pretty good one for an API targeted at TypeScript only. I still believe that imposing such breaking change to everyone would be a bad trade off, but that's the direction I would investigate for a thin wrapper improving type safety for TypeScript users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants