-
Notifications
You must be signed in to change notification settings - Fork 516
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(connectInfiniteHits): fix #2928 #2939
Conversation
@@ -115,7 +118,10 @@ export default function connectInfiniteHits(renderFn, unmountFn) { | |||
results.hits = escapeHits(results.hits); | |||
} | |||
|
|||
hitsCache = [...hitsCache, ...results.hits]; | |||
if (lastReceivedPage < state.page) { |
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'll need to be careful with this line if we ever allow "scrolling up" as in #1972
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 it's blocking for now?
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 think for now it's fine, we just need to keep it in mind if we ever allow scrolling to go the other direction
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 thanks :)
Deploy preview for algolia-instantsearch ready! Built with commit 5581069 https://deploy-preview-2939--algolia-instantsearch.netlify.com |
@@ -54,22 +59,28 @@ describe('connectInfiniteHits', () => { | |||
createURL: () => '#', | |||
}); | |||
|
|||
// test that rendering has been called during init with isFirstRendering = false | |||
expect(rendering.callCount).toBe(2); |
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.
Is it intended to remove this assertion?
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.
No. Good catch.
expect(renderingOptions.hits).toEqual([ | ||
{ objectID: 'a' }, | ||
{ objectID: 'b' }, | ||
]); |
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 can use the same assertion rather than a different one.
expect(rendering).toHaveBeenLastCalledWith(
expect.objectContaining({
hits: [{ objectID: "a" }, { objectID: "b" }],
}),
false
);
expect(rendering).toHaveBeenLastCalledWith( | ||
expect.objectContaining({ | ||
hits: [{ objectID: 'a' }, { objectID: 'b' }], | ||
results: expect.any(Object), |
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 don't need to assert this property it's not relevant to the test.
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 wasn't sure what was the good strategy, that's why it's a bit inconsistent 😅
Did the changes @samouss :) |
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.
fix makes sense to me
When there is a slow network, the stalled search mechanics will trigger a new render with the same content. This has generally no impact. However the infinite builds up a list of all the results, and there was no mechanics in place to check if the same page has been already received.
To test, you can use dev-novel and with the performance set to slow 3g, you can see that there are no duplicates.
Fix #2928
Might also be a patch for #2938 (need confirmation)