Datatable: Fix memoization does not consider column body functions

[thomaslow]

Fixes #8079.

This pull request fixes the issue that the DataTable cell memoization currently does not check whether column body functions were changed. Column body functions may include additional implicit dependencies that lead to incorrect memoization decisions if not checked, see #8079.

In the example in #8079 the event handler onColumnVisibilityChange (which is referenced inside of the column body function) is redefined with every data change, but not propagated to all cells if they are deemed to be not affected by only comparing old and new rowData. Because of that, when a user clicks on a checkbox, the old (memoized) event handler is called that operates on outdated data, causing unexpected behavior.

[melloware]

cc @acc-cassio

[gnawjaren]

Just a small concern: this will disable cellMemo for all columns using a body function unless it’s wrapped in useCallback, which most existing code likely isn’t – even if it would memoize fine. Could that mean a big chunk of the intended performance benefit might get lost silently?

[acc-cassio]

Again, as remarked in other threads: comparing functions as in this implementation is not a good idea if not done properly. Functions are always compared by reference in JS and the comparison as implemented here will return false even if they are identical. To avoid this, you'd need indeed to wrap the function in useCallback as mentioned by @gnawjaren so that the reference of a function is preserved when its dependencies don't change. This would need to be done by the user when creating the DataTable component and would also demand a bit of restructuring of the props inside the lib so to make the proper drilling of the body prop.

[thomaslow]

@gnawjaren Yes, you are correct. On the other hand, you could argue that there might be a lot of existing code that worked before but doesn't work anymore (like #8079), even though the implementation looks perfectly fine on the surface.

@acc-cassio Yes, comparing functions by === is not ideal. Even if the same function (by reference) is provided, the function could still contain side effects, which would cause weird problems with memoization. I think that checking the column body function with === is an acceptable compromise, though. People that need performance will take the time to figure things out and read about memoization in React, which mentions useCallback.

I guess it boils down to the question of performance vs. ease-of-use: do you want average users to be able to use the DataTable component without requiring them to fully understand how memoization works and know every pitfall that comes with that?

Personally, if it were up to me, I would probably disable memoization by default and add a dedicated performance section to the documentation. Even if not, the documentation should be extended, highlighting that cell memoization is used and that there are various pitfalls coming with that when extending a DataTable via templating.

I certainly didn't know about it, given that the online documentation is still on 10.9.1, and my project randomly failed in a very strange way (like #8079) simply by upgrading a minor version of PrimeReact.