[docs] Memoize array sorting

[iammminzzy]

• I have followed (at least) the PR section of the contributing guide.

Address #41709 (review)

[mui-bot]

Netlify deploy preview

https://deploy-preview-42010--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 4f73f1a

[ZeeshanTamboli]

Do we really need to optimize performance with memoization here? The tables have only 15-20 rows, except for the MaterialShowcase demo which has around 45 rows. Most of these useMemo have dependencies that frequently change. Except for when a row is selected or all rows are selected (updating the selected state) in which case, the component would re-render, allowing for reusing the cached rows value. What are your thoughts? @Janpot

[NMinhNguyen]

Cc @michaldudak (#41709 (comment))

For what it’s worth, there’s already a precedent:

material-ui/docs/data/material/components/table/EnhancedTable.tsx

Lines 305 to 311 in ca6b5b4

	const visibleRows = React.useMemo(
	() =>
	[...rows]
	.sort(getComparator(order, orderBy))
	.slice(page * rowsPerPage, page * rowsPerPage + rowsPerPage),
	[order, orderBy, page, rowsPerPage],
	);

so whatever direction this PR goes in should probably be consistent overall.

[Janpot]

Do we really need to optimize performance with memoization here? The tables have only 15-20 rows, except for the MaterialShowcase demo which has around 45 rows.

This is mostly a philosophical argument, but I think in our examples we have the duty to write idiomatic React code. Our community copies and pastes from them and takes inspiration. It is technically true that in the minimal example, memoizing won't move the needle significantly in terms of performance, if it moves at all. But most derivatives of this example will turn out to be vastly more complex. While I don't want to open up the debate between "memoize all" vs. "measure before memoize", I feel like there would be a relatively broad consensus around always memoizing if the operation is >=O(n), especially if the result is not referentially stable. It's a biased viewpoint though, I don't have the bandwidth to engage in a follow-up discussion 🙂.

[ZeeshanTamboli]

Okay, in that case, I'll leave the final decision to official team members like @DiegoAndai or others.

[DiegoAndai]

I usually agree that demos should include only the strictly necessary. @Janpot makes a good point that the useMemo feature will benefit the majority of users' use cases, although this can also be "covered" by adding a callout in the docs about memoization under the demos that might benefit, like these. This would be more educational and better guidance for users than blindly copying useMemo without understanding why, but people might not read it.

I'm not sure what's the better option 😅, but I agree it should be consistent. I would like to know @samuelsycamore's take on this.

[NMinhNguyen]

Apologies for the bump, but would it be possible to agree on the approach here?

[samuelsycamore]

I see valid arguments on both sides, and I think I'd lean more on the side of providing idiomatic React code. If I'm copy+pasting components into my app, I'd rather have the experience of "too much" than "not enough" to assemble a performant MVP that does all of the React-y things I need it to do. And if I do intend to implement useMemo in my component, I'd be relieved to see it already set up in this demo.

In this case, if we move forward with the changes, maybe it would make sense to (briefly) describe the purpose of useMemo and the fact that it's not strictly necessary in a comment right above it in the code?

[NMinhNguyen]

maybe it would make sense to (briefly) describe the purpose of useMemo and the fact that it's not strictly necessary in a comment right above it in the code?

Any suggestions on the wording? :)

[samuelsycamore]

To be clear, I don't think we should memoize everything—but I do see value in adding it to the Joy UI Table "sort and selection" demo for example, which is meant to be closer to a real-world UI, along with some text explaining why it's there and why you might (not) need it.

[DiegoAndai]

I don't think adding useMemo without measuring is idiomatic React code. The React docs mention that measuring is important, and useMemo is useful only in a few cases. Other React educators align with this:

• Kent C. Dodds: https://kentcdodds.com/blog/usememo-and-usecallback#conclusion
• Ryan Florence: https://reacttraining.com/blog/react-inline-functions-and-performance#premature-optimization-is-the-root-of-all-evil

In summary, I wouldn't add useMemo to any demo unless we measure that it's an actual improvement for that demo with the data being used. I would add a callout that it might be useful depending on the use case, and that if they use it they should measure.

[michaldudak]

In principle, premature optimization is generally bad. However, in the case of useMemo, overmemoizing shouldn't be that bad. Otherwise, React Compiler (which memoizes everything it can) wouldn't make much sense.

[ZeeshanTamboli]

@iammminzzy No decision has been made yet. Would you like to close this PR, or do you still want to propose it? I'd recommend closing it, as I don't think it's worth discussing further or involving others right now.

[iammminzzy]

@iammminzzy No decision has been made yet. Would you like to close this PR, or do you still want to propose it? I'd recommend closing it, as I don't think it's worth discussing further or involving others right now.

Alright, I'll close it for now.