-
Notifications
You must be signed in to change notification settings - Fork 529
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
Conversation
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:
|
e1e6a79
to
e306cec
Compare
* 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
cb2eb36
to
d3f16f1
Compare
There was a problem hiding this 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)
packages/instantsearch.js/src/connectors/trending-items/__tests__/connectTrendingItems-test.ts
Outdated
Show resolved
Hide resolved
✅ Approved (I can't do it properly as I'm the creator of the PR). |
There was a problem hiding this 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'; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 }) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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