Closed AdamRamberg closed 8 months ago
The initial benchmarking numbers were way off since the item facets were actually never updating. This was changed in the latest commit. Will update the number in the PR description when the numbers comes in from the CI.
This PR makes #56 obsolete.
Today the
<Map />
component is internally using the array index as a key when rendering out each item. This causes a similar problem to what is discussed in the official React docs. This PR introduces a prop calledkeySelector
, which enables the consumer to select a unique key from the item data.This PR also uses React's
memo
hoc, in order to only re-render items on initial mount and not when re-render all children when the array facet changes.In this PR we kept both implementations to be able to do a side-by-side comparison of the Map component: (old) and
<MapWithKey />
(new)Benchmarking
When comparing the new implementation with the old one they seem to be equivalent. Relative performance listMemoFacet/listMemoState: 53.85 Relative performance listMemoWithKeyFacet/listMemoState: 55.24 Relative performance markerFacet/markerState: 90.88 Relative performance markerWithKeyFacet/markerState: 90.76
Discussion
keySelector
prop be mandatory or optional? The best practice is not to use the array index as a key, but making it mandatory would be a breaking change. If we make it non-mandatory, we could fallback to using indices as key, which will have the same issues as before, but won't force users to add akeySelector
.