search

inveniosoftware / react-searchkit

React component library for interacting with a REST API.
https://inveniosoftware.github.io/react-searchkit/
MIT License
78 stars 40 forks source link

bug: ResultsLoader children multiple instances #101

Closed topless closed 4 years ago

topless commented 4 years ago

All the components that are wrapped inside <ResultsLoader></ResultsLoader> component are constructed twice check ResultsLoader render.

This is due to the fact that our state starts with loading = false, so what happens is

1) initial state loading = false: children get instantiated 2) loading stateloading = true: spinner is shown all good 3) completed state loading = false: children get instantiated (again)

Although changing the initialState to true does the trick, a singleton on children or another approach might serve us better, @zzacharo @ntarocco WDYT?

zzacharo commented 4 years ago

I cannot recall why we had the initialState with loading:false but I think that we should probably change to true as you suggested. I am thinking that is better to show a spinner twice rather than showing things that should not. @topless Out of curiosity, what was the problem because of this?

topless commented 4 years ago

@zzacharo I wasn't looking for anything in particular, I just noticed the constructors in ILS running more than once. I am not even sure if it has any further implications other than downgraded performance.

zzacharo commented 4 years ago

@topless I am not sure is downgrading performance, I mean in a level that is at least significant. I think the loading was set to false, as you would expect that you are not in a loading state when the app loads. When an action is triggered then you transit to that state. Just to summarise that, I don't think is a bug but more like a design decision on your initial state. But I think is fine by me to switch the initial state if also @ntarocco agrees!

topless commented 4 years ago

@zzacharo some of my thoughts on the matter, although you might not notice a staggering performance improvement (on new computers), take into consideration that rendering is expensive. But not rendering multiple times, we will avoid any sort of flickering that might happen and on top of that, we avoid any potential memory leaks from the double instantiation.