feat: implement Recommend Batch Context (RECO-961)#131
Conversation
138de76 to
e92434c
Compare
e92434c to
c74ea3c
Compare
francoischalifour
left a comment
There was a problem hiding this comment.
I reviewed the library aspects but not yet the batching logic.
Sorry for the noise I was using another review tool and meant to send a single review.
| const RecommendComponent = ({ | ||
| objectId, | ||
| model, | ||
| }: { | ||
| objectId: string; | ||
| model: string; | ||
| }) => { | ||
| const { register, hasProvider } = useRecommendContext(); | ||
|
|
||
| React.useEffect(() => { | ||
| const key = JSON.stringify({ | ||
| k: objectId, | ||
| }); | ||
| register({ | ||
| key, | ||
| getParameters: () => { | ||
| return { | ||
| keyPair: { | ||
| key, | ||
| value: 1, | ||
| }, | ||
| queries: [ | ||
| { | ||
| indexName: 'index', | ||
| model, | ||
| objectID: objectId, | ||
| maxRecommendations: 10, | ||
| }, | ||
| ], | ||
| }; | ||
| }, | ||
| onRequest: () => {}, | ||
| onResult: (result) => { | ||
| onResultSpy(result); | ||
| }, | ||
| }); | ||
| }, [register, objectId, model]); | ||
| return ( | ||
| <div> | ||
| <div>hasProvider:{`${hasProvider}`}</div> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
What's the point of this test component? It basically replicates the internal logic our the official Recommend components. Any change there would make this test meaningless.
Can we make sure to test our public API to bring confidence that it works in customers' applications?
There was a problem hiding this comment.
The public API is exposed through the hooks. So that has already been tested.
Since we don't expose internal logic this was my attempt at capturing and testing the life-cycle of the provider. I can't think of another way.
But indeed this is is already tested through the individual hooks.
| expect(mapByScoreToRecommendationsSpy).toHaveBeenCalledTimes( | ||
| mapByScore ? 1 : 0 | ||
| ); | ||
| expect(mapToRecommendationsSpy).toHaveReturnedTimes(mapTo ? 1 : 0); |
There was a problem hiding this comment.
Why test that the internal functions were called and not the actual output? That's an implementation detail that doesn't describe what this test is supposed to do. Reading this test, it's not clear what contract we're testing.
| indexName, | ||
| maxRecommendations, | ||
| threshold, | ||
| // @ts-expect-error |
There was a problem hiding this comment.
Can we provide an explanation after each @ts-expect-error to express why it's needed and to potentially better understand when it can be removed?
There was a problem hiding this comment.
I believe this is the last one left that isn't in a test.
The problem is that RecommendationsQuery from @algolia/recommend doesn't allow to pass a facetName
Which I was meaning to ask you or @sarahdayan about.
Is this an omission from @algolia/recommend ? Or should I define a type that will be basically type RecommendationsQuery & { facetName?:string } ?
There was a problem hiding this comment.
Sarah and I didn't introduce the trending features, Sophie did. It's likely a wrong typing.
There was a problem hiding this comment.
I see, thanks for the clarification! I'll have a look at that type and maybe propose a change to fix the issue.
| }; | ||
| } | ||
|
|
||
| describe('Recommend', () => { |
There was a problem hiding this comment.
What I would expect from this test is:
- render only provided components (without Recommend client) and assert that the correct Recommend client method is called
- render provided components (without Recommend client) and detached components (with Recommend client) within the provider and assert that the correct Recommend client methods are called on the correct clients.
Basically, the tests should explain the contract as they're written in the RFC.
| @@ -0,0 +1,226 @@ | |||
| import { RecommendClient } from '@algolia/recommend'; | |||
There was a problem hiding this comment.
modelling the entire batch logic in React means that if you're ever interested in enabling this for regular JS or another framework, you'll need to entirely rewrite the logic.
There was a problem hiding this comment.
Yes, as far as we know for now. Batch only makes sense in React and that's the only request we got. Otherwise users can just craft their queries manually.
More details can be found in the RFC
There was a problem hiding this comment.
I think it does make sense to only implement this in React for now, but the majority of this code could (and IMHO should) be extracted into a separate purpose-built set of functions that can be used outside of react too. In InstantSearch we've had a significant amount of pain in making parts of the code that only supported InstantSearch.js generic, and I'd recommend that core items don't become part of a specific flavour
There was a problem hiding this comment.
I agree, there's long-term benefits in designing these core parts in an agnostic way. Conceptually, this isn't React specific.
There was a problem hiding this comment.
so will you not address this?
There was a problem hiding this comment.
"Batching" already works in plain JS. We can call client.getRecommendations with all the queries.
For me "batching" in the context of this PR only makes sense if the user is working in a declarative context. So I don't see what can be made more generic / ported to plain JS?
packages/recommend-react/src/__tests__/useTrendingItems.test.tsx
Outdated
Show resolved
Hide resolved
Haroenv
left a comment
There was a problem hiding this comment.
all existing comments still valid as well
|
|
||
| const results: Record<string, BatchRecommendations<TObject>> = {}; | ||
|
|
||
| keys.forEach((keyPair) => { |
There was a problem hiding this comment.
fine as a forEach, but reduce fits this just as well
There was a problem hiding this comment.
or a map and Object.fromEntries
There was a problem hiding this comment.
I know this is a bit subjective, but I found this to be more readable than the more clever reduce approach.
Implement Recommend (Batch) Context
🤔 What
Create a Recommend React Provider and enable batching.
More details at RECO-1400 and
RFC - recommend-react - Recommend Provider & Request batching🤷♂️ Why
Recommend REST API allows for query Batching, we want to make this feature more accessible to users who use the React UI library.
🔍 How
Users who want to take advantage of query Batching can wrap their components with a
<Recommend>provider.Each Recommend UI component will register its query parameters with the parent context and requests will be automatically batched.
To opt a specific component out of batching (using a different index for example) the user can pass a
recommendClientinstance with the appropriate parameters to that particular component.🧪 Testing
Batching can be tested in action by running the React demo project under examples
Check the network tab and see how different recommend queries are batched with a single HTTP request.