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

Add missing peer dependency from algoliasearch-helper #4950

Merged
merged 1 commit into from
Dec 13, 2021

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
@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