Fix #6639: VirtualScroller improve useUpdate comparison

[melloware]

Fix #6639: VirtualScroller compare using deepEquals

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
primereact	⬜️ Ignored (Inspect)	Visit Preview		May 20, 2024 9:47pm
primereact-v9	⬜️ Ignored (Inspect)	Visit Preview		May 20, 2024 9:47pm

[mamsellem]

(copied from comment on the ticket #6639).

I don't think using deepequals is a good idea.
We are only interesteed in updating the bounds of the virtual scroller, so only the dimensions of the items matter, not the items themselves.

This was my fix for this issue:

line 570 of VirtualScroll.js

useUpdateEffect(() => { 
   if (!prevProps.items || prevProps.items.length !== (props.items || []).length
     || (both && prevProps.items[0].length !== (props.items ? props.items[0].length : 0))) {
      init();
 } 

What do you think ?

[melloware]

Yes but your fix presupposes item[0] is an array of things etc. To me that seems riskier than a deep equals where IF the collection changes at all it must re-render?

[mamsellem]

My understanding is that when virtualScroller "both" property is set to true, then items should be an array of arrays, of the same length, so items[0] must be an array.

There is another occurence of items[0] at line 250 of virtualScroller.js:

 const getLast = (last = 0, isCols) => {
            return props.items ? Math.min(isCols ? (props.columns || props.items[0])?.length || 0 : (props.items || []).length, last) : 0;
        };

[melloware]

understood let me look deeper

[melloware]

OK updated, I made it a little more readable and understandable for the next person!

[mamsellem]

Thank you for taking the time to rewrite the code and make it more readable.
A small optimization would be make the calculations only if both is true, but it's really minor.