perf(refinementList): Stop creating URL for hidden refinements. #934
Conversation
|
By analyzing the blame information on this pull request, we identified @vvo, @jerskouille and @bobylito to be potential reviewers |
|
props are frozen so you cannot change them inside render, otherwise: legit change. |
| @@ -127,6 +128,8 @@ class RefinementList extends React.Component { | |||
| } | |||
|
|
|||
| const limit = this.state.isShowMoreOpen ? this.props.limitMax : this.props.limitMin; | |||
| this.props.facetValues = this.props.facetValues.slice(0, limit); | |||
There was a problem hiding this comment.
this is not doable in React, you need to get a new reference instead of modifying props
There was a problem hiding this comment.
What do you mean by not doable? The above code is currently working.
Is that a React best practice?
There was a problem hiding this comment.
Ok my bad, just saw the Travis tests. I'll fix it.
Can't we have an eslint rule to detect that somehow?
There was a problem hiding this comment.
Otherwise running npm test on your machine should have made them fail?
There was a problem hiding this comment.
Nope, tests went through without issues (I have an npm test on pre-push)
There was a problem hiding this comment.
So its weird if they failed on travis, maybe you need to npm install
|
@pixelastic You need some modifications for this PR to be merged |
pixelastic
force-pushed
the
perf/url-refinement-list
branch
from
518014b
to
fa3e8b1
Compare
|
@vvo I've updated the PR to not update the |
|
Hmm the functional tests seems broken, I would say this PR breaks something because the functional tests are looking for [href=Appliances something] and they cannot find it anymore. See the readme for how to launch the functional tests https://github.com/algolia/instantsearch.js#functional-tests |
pixelastic
force-pushed
the
perf/url-refinement-list
branch
2 times, most recently
from
65ca037
to
06feb58
Compare
This actually fixes all my perf issues on the SteamDB demo.
pixelastic
force-pushed
the
perf/url-refinement-list
branch
from
06feb58
to
2cdd17d
Compare
|
I fixed the functional test. I actually had to make the change (passing a method to generate the url instead of the generated url) to each widget that was using the RefinementList component. |
|
looks very nice and clean. Should help a lot performance wise. My mistake for generating the urls upfront in a desperate try to avoid render() calls by doing deep comparisons while since you activate the urlSync, every facet value is different at every user interaction (query is different, url is different, render) |
| ReactDOM.render( | ||
| <RefinementList | ||
| attributeNameKey="path" | ||
| collapsible={collapsible} | ||
| createURL={_createURL} |
There was a problem hiding this comment.
The only "better" optimisation we could do is know upfront that urlSync is deactivated (thus, no different urls per facet values) but I do not see an easy way to do so.
And it may be an edge case we do not need to support.
|
LGTM, I had only one comment, let me know what you think @pixelastic |
|
I say we wait for it to become a bottleneck before fixing it ;) |
This actually fixes all my perf issues on the SteamDB demo.