Skip to content
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

Merged
merged 7 commits into from May 28, 2018
Merged

fix(connectInfiniteHits): fix #2928 #2939

merged 7 commits into from May 28, 2018

Conversation

bobylito
Copy link
Contributor

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)

@@ -115,7 +118,10 @@ export default function connectInfiniteHits(renderFn, unmountFn) {
results.hits = escapeHits(results.hits);
}

hitsCache = [...hitsCache, ...results.hits];
if (lastReceivedPage < state.page) {
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks :)

@algobot
Copy link
Contributor

algobot commented May 23, 2018

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);
Copy link
Contributor

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?

Copy link
Contributor Author

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' },
]);
Copy link
Contributor

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),
Copy link
Contributor

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.

Copy link
Contributor Author

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 😅

@bobylito
Copy link
Contributor Author

Did the changes @samouss :)

Copy link
Contributor

@Haroenv Haroenv left a 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

@bobylito bobylito merged commit 95279d7 into develop May 28, 2018
@bobylito bobylito deleted the fix/2928 branch May 28, 2018 09:04
bobylito added a commit that referenced this pull request May 28, 2018
* refactor(connectInfiniteHits): use jest instead of sinon in tests
* test(connectInfiniteHits): confirm #2928
* test(connectInfiniteHits): remove unsupported option from test
* fix(connectInfiniteHits): fix #2928
* refactor(connectInfiniteHits-test): review from @samouss
bobylito pushed a commit that referenced this pull request May 28, 2018
<a name=2.7.5></a>
## [2.7.5](v2.7.4...v2.7.5) (2018-05-28)

### Bug Fixes

* **clear-all:** apply excludeAttribute correctly with clearsQuery ([#2935](#2935)) ([e782ab8](e782ab8))
* **connectInfiniteHits:** fix [#2928](#2928)  ([#2939](#2939)) ([0293a31](0293a31))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants