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

Type conflict between algoliasearch client and its usage in instantSearch #4916

Closed
bidoubiwa opened this issue Oct 4, 2021 · 5 comments · Fixed by #4938 or #4939
Closed

Type conflict between algoliasearch client and its usage in instantSearch #4916

bidoubiwa opened this issue Oct 4, 2021 · 5 comments · Fixed by #4938 or #4939

Comments

@bidoubiwa
Copy link
Contributor

🐛 Bug description

related to: #4911

The InstantSearch client returns upon search with the geo widget the insideBoundingBox field containing a string.

See the following test:

https://github.com/algolia/instantsearch.js/blob/6e6293046de82f90336bc857efa9d0f5fcb8e62b/src/widgets/geo-search/__tests__/geo-search-test.ts#L1717-L1721

Nonetheless the declared type dictates that it should return an array of array of numbers:

see SearchOptions types.

readonly insideBoundingBox?: ReadonlyArray<readonly number[]>;

🔍 Bug reproduction

Steps to reproduce the behavior:

  1. Create a custom searchClient for instantSearch
  2. Try to adapt insideBoundingBox
  3. TypeError
Argument of type 'readonly (readonly number[])[] | undefined' is not assignable to parameter of type 'string'.

💭 Expected behavior

They should be of same type

Environment

  • "algoliasearch": "^4.10.5",
  • "instantsearch.js": "^4.30.1",
@Haroenv
Copy link
Contributor

Haroenv commented Oct 5, 2021

yes, it's really the client type that is a little more restrictive than needed. Does having a ts-ignore have an impact on any site though?

The correct type is ReadonlyArray<readonly number[]> | `${number},${number},${number},${number}`;, and then the ts-ignore could be ignored, when the algoliasearch requirement is set to the version in which that fix is done

@bidoubiwa
Copy link
Contributor Author

Does having a ts-ignore have an impact on any site though?

It creates a bit of magic code/technical debt as you'll have to remember that somewhere in your code lies a ts-ignore that may create bugs if instantSearch decides to change the type of insindeBoundingBox.

It is the reason I think why you use TypeScript, to avoid manipulating variables in another type.

The correct type is ReadonlyArray<readonly number[]> | ${number},${number},${number},${number};, and then the ts-ignore could be ignored, when the algoliasearch requirement is set to the version in which that fix is done

Thats cool 😊 Is the fix planned for a near release?

@Haroenv
Copy link
Contributor

Haroenv commented Oct 5, 2021

I'd need to check whether the typescript version required in algoliasearch is modern enough to allow template strings, but otherwise, feel free to make a pull request and then it will be fixed @bidoubiwa :)

@bidoubiwa
Copy link
Contributor Author

bidoubiwa commented Oct 5, 2021

I will do that! Thanks a lot for your answers :) I thought you might not want to change the types of algoliasearch-client as they are independent of instantsearch 😅. But If it's oke and if it does not break the whole package, I will absolutely do the PR!

@bidoubiwa
Copy link
Contributor Author

bidoubiwa commented Oct 11, 2021

Should be fixed by: algolia/algoliasearch-client-javascript#1310

Haroenv pushed a commit that referenced this issue Oct 21, 2021
* refactor(version): bump algoliasearch version to v0.11.0

* Import algoliasearch tarball package with type fix

* refactor(packages): Remove algoliasearch tarball

* refactor(types): Remove type work-arround for boundingBox (#2)

* Import algoliasearch client with type fix

* Remove unecessary type enforcement

* refactor(package): Bump algoliasearch to v0.11.0

fixes: #4916
Haroenv added a commit that referenced this issue Nov 16, 2021
* chore(version): bump algoliasearch version to v0.11.0 (#4938)

* refactor(version): bump algoliasearch version to v0.11.0

* Import algoliasearch tarball package with type fix

* refactor(packages): Remove algoliasearch tarball

* refactor(types): Remove type work-arround for boundingBox (#2)

* Import algoliasearch client with type fix

* Remove unecessary type enforcement

* refactor(package): Bump algoliasearch to v0.11.0

fixes: #4916

* chore(version): Fix failing tests when using algoliasearch@v3 (#4941)

* docs(contributing): Add doc on instantsearch cross version testing

* fix(tests): Fixes insideboundingbox tests

* core(geo): Reverse removal of geo forced typecasting

* Update testing script in contributing

* Typescript installation documentation

* chore(version): reverse type enforcing functions (#4942)

* update wordings, remove script

* Apply suggestions from code review

Co-authored-by: Sarah Dayan <5370675+sarahdayan@users.noreply.github.com>

Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>
Co-authored-by: Sarah Dayan <5370675+sarahdayan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants