Skip to content

Conversation

@yacinehmito
Copy link
Contributor

@yacinehmito yacinehmito commented Nov 1, 2021

The dependency algoliasearch-helper declares algoliasearch as a peer dependency [1], but instantsearch.js does not declare it as a dependency nor a peer dependency.
This can cause undefined issues during peer dependency resolution, and triggers warnings when using yarn v2+.

The commit adds algoliasearch as a peer dependency.

@yacinehmito yacinehmito changed the title v2.10.3 Add missing peer dependency from algoliasearch-helper Nov 1, 2021
@yacinehmito yacinehmito marked this pull request as draft November 1, 2021 21:21
@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 1, 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 849aa8a:

Sandbox Source
InstantSearch.js Configuration

The dependency algoliasearch-helper declares algoliasearch as a peer
dependency [1], but instantsearch.js does not declare it as a dependency
not a peer dependency.
This can cause undefined issues during peer dependency resolution, and
triggers warnings when using yarn v2+.

The commit adds algoliasearch as a peer dependency.

[1] https://github.com/algolia/algoliasearch-helper-js/blob/develop/package.json#L103
@yacinehmito yacinehmito marked this pull request as ready for review November 1, 2021 21:28
@Haroenv
Copy link
Contributor

Haroenv commented Nov 3, 2021

With typescript in mind it should probably indeed be a peer dependency, even though it's technically optional still

@tkrugg
Copy link
Contributor

tkrugg commented Dec 10, 2021

I guess we could add it now.
Most users already have algoliasearch installed and won't even see the warning. The range >= 3.1 < 5 is large enough that we remain compatible with both v3 & v4.
@Haroenv can you please outline the downsides of merging this change?

@Haroenv
Copy link
Contributor

Haroenv commented Dec 10, 2021

With npm v7 it breaks their build if they don't have it installed @tkrugg, so adding is a breaking change

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood in which cases this errors. As we have the peer dependency in the helper, this doesn't consist of a breaking change, but just a fix

@Haroenv Haroenv merged commit 468578d into algolia:master Dec 13, 2021
@yacinehmito
Copy link
Contributor Author

Thank you 😄

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.

3 participants