react-dnd / react-dnd

Drag and Drop for React
http://react-dnd.github.io/react-dnd
MIT License
20.98k stars 1.99k forks source link

Array mutation redrawing child components for elements in array #3321

Open a2f0 opened 3 years ago

a2f0 commented 3 years ago

I'm not actually sure if this is a bug, but:

While looking at the official simple sortable example it appears that when I drag an element up and down in the list that all card components re-render when reordering the list instead of the two elements whose positions are being swapped in the array. I can see this by simply putting a console.info('redraw') inside of the Card component as I've done with the fork here.

For large lists, this is causing some performance degradation, presumably due to the re-renders. I was under the impression that using update from immutability-helper in conjunction with useCallback as done in the demo would allow the array to be mutated without having to re-render all child components. Is my understanding incorrect about this? I'm trying to drag elements of a list without all list components re-rendering. Thank you.

simonpkerr commented 3 years ago

the immutability helper library just makes updating state a bit easier, it doesn't have anything to do with rendering. if the state of the cards changes, it'll re-render. that's just react. the docs suggest fixed data table when dealing with large data sets

a2f0 commented 3 years ago

Ah, alright. Thank you @simonpkerr. I think what threw me off is the comment in the example code shown in the attached image. The note explicitly says that the mutation of the monitor item is good for the sake of performance. This is directly underneath moveCard. I guess I assumed moveCard already had a high degree of performance optimization. i.e. I don't see why modifying the item.index would have such a notable performance improvement in comparison to the expensive moveCard which re-renders the entire list anyway. Please feel free to close this issue as you see fit. Thank you for the help.

Screen Shot 2021-10-18 at 10 50 24 AM
simonpkerr commented 3 years ago

Yeah, it just seems to set the index to the current hover index so that on the next render, the first condition is met (drag index = hover index) so it immediately returns rather than calling moveCard. It’s a shame the actual example doesn’t work.

On 18 Oct 2021, at 15:57, a2f0 @.***> wrote:

 Ah, alright. Thank you @simonpkerr. I think what threw me off is the comment in the example code shown in the attached image. The note explicitly says that the mutation of the monitor item is good for the sake of performance. This is directly underneath moveCard. I guess I assumed moveCard already had a high degree of performance optimization. i.e. I don't see why modifying the item.index would have such a notable performance improvement in comparison to the expensive moveCard which re-renders the entire list anyway. Please feel free to close this issue as you see fit. Thank you for the help.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

a2f0 commented 3 years ago

Thank you for addressing my confusion, @simonpkerr. I appreciate your expertise and attention.