typesense / typesense-instantsearch-adapter

A JS adapter library to build rich search interfaces with Typesense and InstantSearch.js
MIT License
414 stars 64 forks source link

Typescript definitions missing fields #91

Closed themerk closed 2 years ago

themerk commented 2 years ago

Description

I'm very new to Typescript, so maybe I'm doing something wrong here. But it seems that the type definition for additionalSearchParameters is missing the fields groupBy and groupLimit. Also, numTypos can actually accept a list of numbers in the URL param, but its type definition is only a single number.

I also needed to @ts-ignore this line:

const search = instantsearch({
    // @ts-ignore
    searchClient: searchClient,
    ....
});

Because Typesense's searchClient is defined only as an object, but the definitions in instantsearch (which I think they only recently added?) require it to actually have the right fields.

Really appreciate the work in adding type definitions though, I'm brand new to Typescript but already enjoying it! Thanks!

Workaround

Other than the @ts-ignore line, I used this code to amend the type definitions:

declare module 'typesense-instantsearch-adapter' {
  interface BaseSearchParameters {
    groupBy?: string
    groupLimit?: number
  }

Metadata

typesense-instantsearch-adapter Package Version: 2.3.0 typesense Package Version: 1.1.3 instantsearch.js** Package Version: 4.37.3

jasonbosco commented 2 years ago

@themerk Thank you for catching this. I've pushed out a fix for this in 2.3.1-0. Could you give it a shot now?

Hopefully issues like this will go away once we implement #85.

themerk commented 2 years ago

Thanks for the quick response! It's definitely better. Two suggestions:

Until #85 is done, just make the SearchClient any type, then the @ts-ignore workaround isn't needed. Otherwise it's potentially very confusing for someone new to the library

https://github.com/typesense/typesense-instantsearch-adapter/blob/4c7a707ad636a553c52d7eaf9ce394ab56ea4775/index.d.ts#L1

Allow number or string for numTypos, otherwise this release may break existing code https://github.com/typesense/typesense-instantsearch-adapter/blob/4c7a707ad636a553c52d7eaf9ce394ab56ea4775/index.d.ts#L25

themerk commented 2 years ago

Actually, may as well just replace that first line in index.d.ts with this:

import type {SearchClient} from 'algoliasearch';
jasonbosco commented 2 years ago

@themerk I've gone ahead and implemented #85, which should solve the num_typos issue as well. Could you upgrade to v2.4.0-0 of the adapter (and v1.2.0-0 of typesense.js if you have a direct dependency) and let me know how it goes?

Actually, may as well just replace that first line in index.d.ts with this:

import type {SearchClient} from 'algoliasearch';

This would add algoliasearch as a dependency to the typesense adapter, which I'd prefer to not introduce just to add types. I've set the type for search client to any for now. Does that solve the Typescript warning?

themerk commented 2 years ago

Awesome! Looks good. SearchClient warning and num_typos are now fixed.

This would add algoliasearch as a dependency to the typesense adapter

Ohh fair enough, I didn't realise you didn't have that dependency already.

Only very minor thing I've spotted is BaseSearchParameters.prefix shouldn't be deprecated (it's not affected by the change to snake casing)

jasonbosco commented 2 years ago

Ah good catch. I've fixed it in 2.4.0-1. Could you try now?

themerk commented 2 years ago

Yep all good now, thanks!