-
Notifications
You must be signed in to change notification settings - Fork 46
Conversation
'attributesToSnippet', | ||
'hitsPerPage', | ||
'restrictSearchableAttributes', | ||
'snippetEllipsisText' // FIXME remove this line once the engine is fixed. |
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.
Unlike the doc, snippetEllipsisText
is throwing an error. I've reported it to the team, but for now let's have it here.
findAnswers
index.d.ts
Outdated
queryLanguages: string[]; | ||
nbHits: number; | ||
}): Promise<{ | ||
hits: Array<any & { |
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.
can you not use the return type of findAnswers on the client?
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.
this method likely should be generic, like the client one
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.
I've updated the dependency and now it's using the type from the client:
da8af13
src/algoliasearch.helper.js
Outdated
/** | ||
* @typedef Answer | ||
* @type {object} | ||
* @property {string} extract the extracted value with highlights | ||
* @property {string} extractAttribute the attribute used to extract the answer | ||
* @property {number} score the score indicating how well it was matched | ||
*/ | ||
|
||
/** | ||
* @typedef AnswerHit | ||
* @type {object} | ||
* @property {Answer} _answer the object describing why the hit was chosen | ||
*/ | ||
|
||
/** | ||
* @typedef AnswersResult | ||
* @type {object} | ||
* @property {AnswerHit[]} hits the answer hits | ||
*/ | ||
|
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.
these docs aren't used as explanation while there's the definition file (only locally), so I don't think it's needed.
Are you allowed to omit the return type all together or does eslint not allow it?
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.
What do you think? 5aa1206
src/algoliasearch.helper.js
Outdated
'search for answers was called, but this client does not have a function client.initIndex' | ||
); | ||
} | ||
var index = this.client.initIndex(derivedState.index); | ||
if (typeof index.findAnswers !== 'function') { | ||
throw new Error( | ||
'search for answers was called, but this client does not have a function client.initIndex(index).findAnswers' |
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.
this is helpful, but the second error message gives pretty much the same information. Could it be consolidated to save space? How do we do it in searchForFacetValues?
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.
Unlike SFFV, the existence of findAnswers
can be checked only after creating index
here. So I couldn't merge them in a single conditional block.
2433fb1
Co-authored-by: Haroen Viaene <[email protected]>
bba57db
to
7bec235
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.
LGTM!
* feat(answers): add searchForAnswers WIP * update parameters * add a FIXME comment * accept options as an object * add jsdoc and typings * add test cases * fix lint error * better types * clean up error message * Update src/algoliasearch.helper.js Co-authored-by: Haroen Viaene <[email protected]> * remove jsdoc * fix type Co-authored-by: Haroen Viaene <[email protected]>
* feat(answers): add (algolia/algoliasearch-helper-js#804) algolia/algoliasearch-helper-js@da388c2
This PR adds
findAnswers
method.In this way, we can apply facets to /answers call.
It doesn't emit
'result'
but returns a promise of result.If we can replace the traditional hits with the hits from answers completely, then we should go for emitting
'result'
, but it's not the case at the moment. So I chose to return a promise.Let me know what you think.
(I haven't added any tests because we haven't agreed on how to implement this in helper yet. Once decided here, I will add test.)