-
Notifications
You must be signed in to change notification settings - Fork 223
feat(recommend): Add trending types and methods #1396
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
Changes from 6 commits
36bbcfc
727c787
b1f7794
d64940d
4014f29
0d304e6
3567914
a1d7636
cfe8fc5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,7 +105,7 @@ | |
}, | ||
{ | ||
"path": "packages/recommend/dist/recommend.umd.js", | ||
"maxSize": "4.1KB" | ||
"maxSize": "4.2KB" | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
import { TestSuite } from '../../../client-common/src/__tests__/TestSuite'; | ||
|
||
const recommend = new TestSuite('recommend').recommend; | ||
|
||
function createMockedClient() { | ||
const client = recommend('appId', 'apiKey'); | ||
jest.spyOn(client.transporter, 'read').mockImplementation(() => Promise.resolve()); | ||
|
||
return client; | ||
} | ||
|
||
describe('getTrendingFacets', () => { | ||
test('builds the request', async () => { | ||
const client = createMockedClient(); | ||
|
||
await client.getTrendingFacets( | ||
[ | ||
{ | ||
indexName: 'products', | ||
facetName: 'company', | ||
}, | ||
], | ||
{} | ||
); | ||
|
||
expect(client.transporter.read).toHaveBeenCalledTimes(1); | ||
expect(client.transporter.read).toHaveBeenCalledWith( | ||
{ | ||
cacheable: true, | ||
data: { | ||
requests: [ | ||
{ | ||
indexName: 'products', | ||
model: 'trending-facets', | ||
facetName: 'company', | ||
threshold: 0, | ||
}, | ||
], | ||
}, | ||
method: 'POST', | ||
path: '1/indexes/*/recommendations', | ||
}, | ||
{} | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { TestSuite } from '../../../client-common/src/__tests__/TestSuite'; | ||
|
||
const recommend = new TestSuite('recommend').recommend; | ||
|
||
function createMockedClient() { | ||
const client = recommend('appId', 'apiKey'); | ||
jest.spyOn(client.transporter, 'read').mockImplementation(() => Promise.resolve()); | ||
|
||
return client; | ||
} | ||
|
||
describe('getTrendingItems', () => { | ||
test('builds the request', async () => { | ||
const client = createMockedClient(); | ||
|
||
await client.getTrendingItems( | ||
[ | ||
{ | ||
indexName: 'products', | ||
}, | ||
], | ||
{} | ||
); | ||
|
||
expect(client.transporter.read).toHaveBeenCalledTimes(1); | ||
expect(client.transporter.read).toHaveBeenCalledWith( | ||
{ | ||
cacheable: true, | ||
data: { | ||
requests: [ | ||
{ | ||
indexName: 'products', | ||
model: 'trending-items', | ||
threshold: 0, | ||
}, | ||
], | ||
}, | ||
method: 'POST', | ||
path: '1/indexes/*/recommendations', | ||
}, | ||
{} | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import { MethodEnum } from '@algolia/requester-common'; | ||
|
||
import { BaseRecommendClient, TrendingFacetsQuery, WithRecommendMethods } from '../types'; | ||
|
||
type GetTrendingFacets = ( | ||
base: BaseRecommendClient | ||
) => WithRecommendMethods<BaseRecommendClient>['getTrendingFacets']; | ||
|
||
export const getTrendingFacets: GetTrendingFacets = base => { | ||
return (queries: readonly TrendingFacetsQuery[], requestOptions) => { | ||
const requests: readonly TrendingFacetsQuery[] = queries.map(query => ({ | ||
...query, | ||
model: 'trending-facets', | ||
// The `threshold` param is required by the endpoint to make it easier | ||
// to provide a default value later, so we default it in the client | ||
// so that users don't have to provide a value. | ||
shortcuts marked this conversation as resolved.
Show resolved
Hide resolved
|
||
threshold: query.threshold || 0, | ||
})); | ||
|
||
return base.transporter.read( | ||
{ | ||
method: MethodEnum.Post, | ||
path: '1/indexes/*/recommendations', | ||
data: { | ||
requests, | ||
}, | ||
cacheable: true, | ||
}, | ||
requestOptions | ||
); | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import { MethodEnum } from '@algolia/requester-common'; | ||
|
||
import { BaseRecommendClient, TrendingItemsQuery, WithRecommendMethods } from '../types'; | ||
|
||
type GetTrendingItems = ( | ||
base: BaseRecommendClient | ||
) => WithRecommendMethods<BaseRecommendClient>['getTrendingItems']; | ||
|
||
export const getTrendingItems: GetTrendingItems = base => { | ||
return (queries: readonly TrendingItemsQuery[], requestOptions) => { | ||
const requests: readonly TrendingItemsQuery[] = queries.map(query => ({ | ||
...query, | ||
model: 'trending-items', | ||
// The `threshold` param is required by the endpoint to make it easier | ||
// to provide a default value later, so we default it in the client | ||
// so that users don't have to provide a value. | ||
threshold: query.threshold || 0, | ||
})); | ||
|
||
return base.transporter.read( | ||
{ | ||
method: MethodEnum.Post, | ||
path: '1/indexes/*/recommendations', | ||
data: { | ||
requests, | ||
}, | ||
cacheable: true, | ||
}, | ||
requestOptions | ||
); | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -1 +1,2 @@ | ||||||||||
export type RecommendModel = 'related-products' | 'bought-together'; | ||||||||||
export type RecommendModel = 'related-products' | 'bought-together' | TrendingModel; | ||||||||||
export type TrendingModel = 'trending-items' | 'trending-facets'; | ||||||||||
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry D: #1399 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ export type RecommendationsQuery = { | |
/** | ||
* The `objectID` of the item to get recommendations for. | ||
*/ | ||
readonly objectID: string; | ||
readonly objectID?: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is omitted actually, how come it's still needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we still remodel the code so that types are accurate? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So after a third thought, I wanted to try to make it optional here but required for RP and FBT using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which bring me to a question after a discussion with @PLNech: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Put differently: was
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll ask the opposite question then: what's the purpose of having a specialized method for each model rather than just using |
||
|
||
/** | ||
* Threshold for the recommendations confidence score (between 0 and 100). Only recommendations with a greater score are returned. | ||
|
@@ -38,4 +38,14 @@ export type RecommendationsQuery = { | |
* Additional filters to use as fallback when there aren’t enough recommendations. | ||
*/ | ||
readonly fallbackParameters?: RecommendSearchOptions; | ||
|
||
/** | ||
* Used for trending model | ||
*/ | ||
readonly facetName?: string; | ||
|
||
/** | ||
* Used for trending model | ||
*/ | ||
readonly facetValue?: string; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
import { RecommendationsQuery } from './RecommendationsQuery'; | ||
|
||
export type RelatedProductsQuery = Omit<RecommendationsQuery, 'model'>; | ||
export type RelatedProductsQuery = Omit< | ||
RecommendationsQuery, | ||
'model' | 'facetName' | 'facetValue' | ||
> & | ||
Required<Pick<RecommendationsQuery, 'objectID'>>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
import { RecommendationsQuery } from './RecommendationsQuery'; | ||
|
||
export type TrendingFacetsQuery = Omit<RecommendationsQuery, 'model' | 'facetValue' | 'objectID'> & | ||
Required<Pick<RecommendationsQuery, 'facetName'>>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import { RecommendationsQuery } from './RecommendationsQuery'; | ||
|
||
export type TrendingItemsQuery = Omit<RecommendationsQuery, 'model' | 'objectID'>; |
Uh oh!
There was an error while loading. Please reload this page.