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(client): support v5 fully #6270

Merged
merged 4 commits into from
Aug 20, 2024
Merged

Conversation

dhayab
Copy link
Member

@dhayab dhayab commented Jul 8, 2024

Summary

This PR updates all algoliasearch dependencies in examples to algoliasearch@5 and ensures full compatibility locally with algoliasearch v5

FX-2922

fixes algolia/algoliasearch-client-javascript#1537
fixes #6329

Copy link

codesandbox-ci bot commented Jul 8, 2024

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 106aa8b:

Sandbox Source
example-instantsearch-getting-started Configuration
example-react-instantsearch-getting-started Configuration
example-react-instantsearch-next-app-dir-example Configuration
example-react-instantsearch-next-routing-example Configuration
example-vue-instantsearch-getting-started Configuration

@Haroenv Haroenv force-pushed the chore/examples-algoliasearch-v5 branch from e1e6a79 to e306cec Compare July 17, 2024 08:13
@Haroenv Haroenv changed the title chore(examples): update search client to v5 fix(client): support v5 fully Aug 19, 2024
@Haroenv Haroenv marked this pull request as ready for review August 19, 2024 14:39
dhayab and others added 3 commits August 19, 2024 16:59
* chore: change type

* WIP: expand search parameters

doesn't work as there still are v4 v5 differences of course

* do it correctly

* use regular client

* not sure if this should stay

* WIP

* changes rule

* fix dependency

* change order to make script work

* almost all types fixed

* improve test

* fix(types): accepts client with wrong types

will be fixed once algolia/api-clients-automation#3357 is done

* tests

* chore(helper): compatibility

* chore(legacy): correct replacement

* don't remove v5 fully

* fix errors

* safer (or less safe lol)

* correct import

* fix fake type

* v5 only works

* tests

* script for examples too

* script for examples too

* chore(answers): bleh get rid of this!

* stuff

* simplify
* ci: add downgrade script for algoliasearch v4 and steps

* test:ci instead

* store results
@Haroenv Haroenv force-pushed the chore/examples-algoliasearch-v5 branch from cb2eb36 to d3f16f1 Compare August 19, 2024 15:09
@Haroenv Haroenv marked this pull request as draft August 19, 2024 15:09
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.

most changes are copy-paste dependency changes, bar some type changes in helper to make it properly compatible, as well as some smaller consistency Hit/AlgoliaHit changes (because recommend is now correctly typed without position and query id)

@Haroenv Haroenv marked this pull request as ready for review August 19, 2024 15:19
@Haroenv Haroenv requested review from a team, aymeric-giraudet and shaejaz and removed request for a team August 19, 2024 15:19
@dhayab
Copy link
Member Author

dhayab commented Aug 19, 2024

✅ Approved (I can't do it properly as I'm the creator of the PR).

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

fully trusting @dhayab

@@ -1,5 +1,5 @@
/* global moment Calendar $ */
import algoliasearch from 'algoliasearch/lite';
import { liteClient as algoliasearch } from 'algoliasearch/lite';
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should provide this name as the export?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you're doing any change, adding the default export would be very handy

@@ -9,7 +9,7 @@
},
"browserslist": "firefox 68, chrome 78, IE 11",
"dependencies": {
"algoliasearch": "4.23.2",
"algoliasearch": "5.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

There's 5.0.2 that fixes the highlight and snippet result recursive stuff

Copy link
Contributor

Choose a reason for hiding this comment

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

just an example so we can update separately of this pr

@@ -1598,6 +1598,7 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/refinement-

const helper = jsHelper(
createSearchClient({
// @ts-ignore v5 does not have this method, but it's easier to have it here. In a future version we can replace this method and its usages with search({ type: 'facet })
Copy link
Member

Choose a reason for hiding this comment

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

we have a wrapped named searchForFacets purely for typing purposes if that helps

Copy link
Contributor

Choose a reason for hiding this comment

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

the comment is more for the /lite in v5 which doesn't have sffv anymore, and the order which is checked in InstantSearch is client.sffv, client.initIndex?.sffv, client.search, and so as we're in v5 here technically we should only mock client.search, but that's ok

@@ -472,6 +472,7 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/voice-searc
new SearchParameters({
ignorePlurals: true,
removeStopWords: true,
// @ts-ignore we send optionalWords as a string
optionalWords: 'query',
Copy link
Member

Choose a reason for hiding this comment

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

are both possible? if so, we can fix the spec

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it's allowed in the api, but I think that's only because any array is also allowed to be a string

Copy link
Member

Choose a reason for hiding this comment

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

ok ok so let's keep it like that then

@@ -106,14 +106,15 @@ const connectVoiceSearch: VoiceSearchConnector = function connectVoiceSearch(
const queryLanguages = language
? [language.split('-')[0]]
: undefined;
// @ts-ignore queryLanguages is allowed to be a string, not just an array
Copy link
Member

Choose a reason for hiding this comment

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

same here if you confirm I can fix the spec

Copy link
Contributor

Choose a reason for hiding this comment

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

same I believe it's allowed in the api just because any array is also allowed to be a string, I'd rather leave it to be an array always and eventually change it in InstantSearch

@Haroenv Haroenv merged commit c3b5e80 into master Aug 20, 2024
14 checks passed
@Haroenv Haroenv deleted the chore/examples-algoliasearch-v5 branch August 20, 2024 07:41
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.

Incorrect typing of ConfigureConnectorParams No clear migration guide for v5 and no explicit typing
4 participants