search

mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.73k stars 31.51k forks source link

[docs] Memoize array sorting #42010

Open iammminzzy opened 1 week ago

iammminzzy commented 1 week ago

Address https://github.com/mui/material-ui/pull/41709#pullrequestreview-1991071497

mui-bot commented 1 week ago

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 :no_entry_sign: dangerJS against 4f73f1aaa18d3cb5d58e416df6a36ac519f465f7

NMinhNguyen commented 1 week ago

Cc @michaldudak (https://github.com/mui/material-ui/pull/41709#discussion_r1565526883)

For what it’s worth, there’s already a precedent: https://github.com/mui/material-ui/blob/ca6b5b4fd59e697193261b7f96829bfc6a7b6082/docs/data/material/components/table/EnhancedTable.tsx#L305-L311

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

Janpot commented 1 week ago

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 commented 1 week ago

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

DiegoAndai commented 1 week ago

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.