feat(recommend): Add trending types and methods#1396
Conversation
| * The `objectID` of the item to get recommendations for. | ||
| */ | ||
| readonly objectID: string; | ||
| readonly objectID?: string; |
There was a problem hiding this comment.
I didn't want to break the existing implementation for related-products and bought-together. So I put it optional but it is still mandatory for these models :/
There was a problem hiding this comment.
it should be required, but omitted from the new items then probably, as how facetName is omitted for fbt
There was a problem hiding this comment.
It is omitted actually, how come it's still needed?
There was a problem hiding this comment.
the problem if I do that is with getRecommendations . getRecommendation is used for every get method of Recommend and the objectID will be mandatory even for trends
There was a problem hiding this comment.
Can we still remodel the code so that types are accurate?
There was a problem hiding this comment.
So after a third thought, I wanted to try to make it optional here but required for RP and FBT using Required in their own Query type.
I did that to be able to use getRecommendations for all models.
There was a problem hiding this comment.
Which bring me to a question after a discussion with @PLNech:
What is the purpose of getRecommendation? Does it make sense to make it retrieve recommendations for all kind of model? @francoischalifour maybe you'll know?
There was a problem hiding this comment.
Put differently: was getRecommendation a hack while we defined the actual model names before GA, or is it a feature designed to allow using not-yet-public new models?
- If the former, then it might be time we drop this generic method (which brings subpar "common denominator" DX compared to specialized methods) and only implement Trending as a new, fully-typed method
- If the latter, we'll need to find a generic implementation that is least disruptive to current users.
There was a problem hiding this comment.
getRecommendations() is useful for the situation we're in right now: users can already use the trending model, without it being officially supported with a method in the client.
We didn't have a team to maintain this Recommend client initially so we provided it as an "escape hatch" if we're not reactive to support new models in the client.
getRecommendations() was just the very initial implementation that satisfied us at the time, feel free to change it as you need. And if it's not helpful for the new model, don't feel like you need to use it. (In the future, we can also deprecate it.)
There was a problem hiding this comment.
I'll ask the opposite question then: what's the purpose of having a specialized method for each model rather than just using getRecommendations()? It's basically a multi-query endpoint, and I think it has its uses (like making a single call to retrieve both trending items and trending facets for example). While calling getRelatedProducts() as a multi-query is strange IMO.
|
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 cfe8fc5:
|
| * The `objectID` of the item to get recommendations for. | ||
| */ | ||
| readonly objectID: string; | ||
| readonly objectID?: string; |
There was a problem hiding this comment.
it should be required, but omitted from the new items then probably, as how facetName is omitted for fbt
SophieManley03
force-pushed
the
feat/recommend-trending
branch
from
931ff60 to
c951aae
Compare
SophieManley03
force-pushed
the
feat/recommend-trending
branch
from
c951aae to
727c787
Compare
…earch-client-javascript into feat/recommend-trending
shortcuts
left a comment
shortcuts
left a comment
There was a problem hiding this comment.
It looks good to me!
Only one non-blocking comment if @francoischalifour have nothing agains it
…earch-client-javascript into feat/recommend-trending
SophieManley03
force-pushed
the
feat/recommend-trending
branch
from
bd20da2 to
cfe8fc5
Compare
PLNech
left a comment
PLNech
left a comment
There was a problem hiding this comment.
LGTM! Let's see how it goes once implemented in @algolia/recommend 🙃
| export type RecommendModel = 'related-products' | 'bought-together' | TrendingModel; | ||
| export type TrendingModel = 'trending-items' | 'trending-facets'; |
There was a problem hiding this comment.
| export type RecommendModel = 'related-products' | 'bought-together' | TrendingModel; | |
| export type TrendingModel = 'trending-items' | 'trending-facets'; | |
| export type TrendingModel = 'trending-items' | 'trending-facets'; | |
| export type RecommendModel = 'related-products' | 'bought-together' | TrendingModel; |
|
Thank you all for the reviews 🙇♀️ |
6 participants
What?
Prepare typing and methods for the new recommend model named
trending.Why?
We want to release trending model in beta on March, 10th. In order to have a successful release we would like to update the recommend library so that use can use trending with it.
How?
Here are the specs for trending. It will give a better idea of what is implemented in the search.
We have 3 types of trending:
We don't want to break the current implementation of recommend for our customers.