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

Avoid to rely on shouldComponentUpdate in component #3056

Closed
samouss opened this issue Aug 8, 2018 · 3 comments
Closed

Avoid to rely on shouldComponentUpdate in component #3056

samouss opened this issue Aug 8, 2018 · 3 comments
Assignees

Comments

@samouss
Copy link
Contributor

samouss commented Aug 8, 2018

Describe the bug 🐛

The stats widget re-renders only when the nbHits or processingTimeMS changes. It can lead to issue when you are using the page number in the template for example. When you click on the next page with the Pagination widget only the page value changed not the other ones. The shouldComponentUpdate (sCU) function of the widget will return false and so prevent the re-render.

https://github.com/algolia/instantsearch.js/blob/33fa09835df8678085cb7f382b4973eaa1f9b20f/src/components/Stats/Stats.js#L9-L14

To Reproduce 🔍

  1. Go to https://codesandbox.io/s/l51or5k9lz
  2. Click on the pagination
  3. Stats widget is not updated

Expected behavior 💭

We should trigger the re-render. The quick fix is to add more values inside the condition. But do we really need the sCU hook on those components? To have a better idea we can track the effect on the performance with / without the hook.

@samouss samouss changed the title Avoid to rely on SCU in widget Avoid to rely on SCU in component Aug 8, 2018
@bobylito
Copy link
Contributor

What is SCU @samouss ?

@samouss samouss changed the title Avoid to rely on SCU in component Avoid to rely on shouldComponentUpdate in component Aug 10, 2018
@samouss
Copy link
Contributor Author

samouss commented Aug 10, 2018

I've update the title and the description.

@bobylito
Copy link
Contributor

I'd go for removing the hook, as it is a very straightforward solution. If we find evidence that this particular widget is causing performance issues, we can work on it then.

@bobylito bobylito self-assigned this Aug 17, 2018
bobylito pushed a commit that referenced this issue Aug 20, 2018
This removes the implementation of shouldComponentUpdate. This can be
seen as a regression but given that it's a pretty simple widget, the
impact shouldn't be noticeable.

Fix #3056
bobylito added a commit that referenced this issue Aug 20, 2018
This removes the implementation of shouldComponentUpdate. This can be seen as a regression but given that it's a pretty simple widget, the impact shouldn't be noticeable.

Fix #3056
bobylito pushed a commit that referenced this issue Sep 10, 2018
<a name=2.10.2></a>
## [2.10.2](v2.10.1...v2.10.2) (2018-09-10)

### Bug Fixes

* **searchbox:** Add missing color to searchbox input field ([#3086](#3086)) ([62b852a](62b852a)), closes [#3075](#3075)
* **Stats:** let the widget render on all values ([#3070](#3070)) ([cd8f17e](cd8f17e)), closes [#3056](#3056)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants