[Performance] Refactor SidebarLinks to not use getDerivedStateFromProps() and other static methods

[marcaaron]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Coming from https://github.com/Expensify/App/pull/7907/files#r816376801

Code cleanup

The file here seems to be following some practices that we should avoid because they are very uncommon in this codebase:

App/src/pages/home/sidebar/SidebarLinks.js

Lines 165 to 174 in 00805b2

	static getDerivedStateFromProps(nextProps, prevState) {
	const shouldReorder = SidebarLinks.shouldReorder(nextProps, prevState.orderedReports, prevState.currentlyViewedReportID, prevState.unreadReports);
	const recentReports = SidebarLinks.getRecentReports(nextProps);
	const orderedReports = shouldReorder
	? recentReports
	: _.map(prevState.orderedReports,
	orderedReport => _.chain(recentReports)
	.filter(recentReport => orderedReport.reportID === recentReport.reportID)
	.first()
	.value());

There are some well documented reasons why we should use this and not here:

https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html

And it looks like we just want to perform a side effect where when various props change we update the reports we display in the sidebar. But for some reason have used derived state to do this instead of something like componentDidUpdate().

Finding this component pretty hard to understand in general...

Solution

1. Clean this up and make it use componentDidUpdate()
2. Put something in the style guide or eslint rule to forbid this practice

• Be sure to complete this TODO

[ctkochan22]

Taking up your offer on letting me steal this :)

[parasharrajat]

There is one more component that uses it. PopoverWithMeasuredContent.

But I think it is needed there. The popover render function is expensive for EmojiPicker and using it prevents render until we are fully ready. I don't exactly remember why I used it there but there was a race condition that I was trying to overcome.

[marcaaron]

Thanks @parasharrajat. This issue is just for SidebarLinks and not all usages of getDerivedStateFromProps(). But I do think we should treat it as a potential anti-pattern and call attention to it if ever observed (hence why adding something the style guide would be good).

[parasharrajat]

Ok. Great.

[ctkochan22]

Punting

[melvin-bot]

@marcaaron, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

[tgolen]

Work is coming along here nicely. Getting a lot of unit tests written

[trjExpensify]

Added this App issue to the WhatsApp Quality project to keep track of it.

[tgolen]

Got the first PR done which optimizes some of the current code which figures out the order reports should be displayed in. My second PR refactors the sidebar to render items more efficiently.

[tgolen]

The last PR is in final stages of review and testing. It should merge this week!

[tgolen]

This is all done