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

fix(insideBoundingBox): allow type to be a string too #1310

Merged
merged 1 commit into from
Oct 20, 2021
Merged

fix(insideBoundingBox): allow type to be a string too #1310

merged 1 commit into from
Oct 20, 2021

Conversation

bidoubiwa
Copy link
Contributor

@bidoubiwa bidoubiwa commented Oct 8, 2021

This PR adds the possibility for the insideBoundingBox type to be a string as well. The reason behind this improvement comes from the usage of insideBoundingBox in instantsearch.js where it is used as a string.

More details in this issue in instantsearch.js.

I suppose this PR adds breaking changes as functions using insideBoundingBox with the sole expectation of it to be ReadonlyArray<readonly number[]> will have to be updated to check if it is one of both.

Here is an example of how it would be breaking:

type InsideBoundingBox = ReadonlyArray<readonly number[]> | string

function myFunction(insideBoundingBox: InsideBoundingBox) {
   insideBoundingBox.map(...) // TypeScript error as Type can now be string.
} 

( I searched to point this PR to v5 since it introduces breaking changes but could not find the related branch 😕)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 8, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit de46145:

Sandbox Source
javascript-client-app Configuration

@Haroenv Haroenv changed the title fix(BREAKING CHANGE): Fix type conflict with instantsearch fix(insideBoundingBox): allow type to be a string too Oct 11, 2021
@Haroenv
Copy link
Contributor

Haroenv commented Oct 11, 2021

If this is changed, what impact does this have on InstantSearch.js for example, does it make the ts-ignore unneeded or does the parameter now need to be checked @bidoubiwa ?

@bidoubiwa
Copy link
Contributor Author

I packed the package and tried it in instantSearch.

I works great! See https://github.com/bidoubiwa/instantsearch.js/pull/1

I also implemented in another PR the simplification it brings in the code of instantSearch where the type does not require to be enforced anymore. see: https://github.com/bidoubiwa/instantsearch.js/pull/2/files

@Haroenv Haroenv merged commit 87d5b0d into algolia:master Oct 20, 2021
@bidoubiwa
Copy link
Contributor Author

Once it is released and the package is updated in instantsearch.je should I make the other pr's as well? Or do you prefer handling it when bumping the version? Both solution (or none) are good to me :)

@Haroenv
Copy link
Contributor

Haroenv commented Oct 20, 2021

I'll release here now, updating the version bump on InstantSearch' side would definitely be welcome, thanks @bidoubiwa !

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

Successfully merging this pull request may close these issues.

None yet

2 participants