Improve RoomList render time when filtering

[germain-gg]

Improves element-hq/element-web#14750
God speed unleashed when used with element-hq/element-web#16969

Benchmark below

• Figure 1: After my improvements
• Figure 2: Before my improvements

• A few events were not cleaned up properly
• Some work is done is the constructor when it could be offloaded at a later stage
• Mounting and unmounting RoomSublist is quite expensive. It ends up being much cheaper to hide rather than unmounting it when empty. Trading a tiny bit of ram for faster filtering

[turt2live]

It also looks like there's failing end to end tests, which look like legitimate failures

[turt2live]

is it actually more performant to move things to componentDidMount? Over the last couple years we've started using the constructor more and more, slowly getting rid of componentDidMount - putting aside arguments about whether that is correct behaviour, I'm curious if there's a performance impact.

[germain-gg]

It depends, generally speaking there can be small performance improvements when the component is rendered but not mounted

But in my opinion the real power of this is that you colocate all your event listeners, if you have some event listeners that you need to place on DOM elements, you will have to perform that in componentDidMount. And on top of that if it is immediatly followed by componentWillUnmount you're one step closer to remembering to clean up your listeners

It also helps in tests where you shallow render your components

Overall moving away from lifecycles makes sense, this is pretty much what the React team has been advocating for, but their solution seems to be using hooks instead

[turt2live]

I'm not sure how to interpret the screenshot: it looks like we're spending more time in code?

Overall this looks sensible, but I am curious how it interacts with the tab indexing/accessibility. It'd also be good to get a comment somewhere in the code to say we deliberately keep the sublists rendered so we don't accidentally optimize it back out 😅

[germain-gg]

I'm not sure how to interpret the screenshot: it looks like we're spending more time in code?

I've read my description again, and it is utterly confusing, the left figure is AFTER my improvements. and the right figure is BEFORE 👀

Jonathan (jboi) also took a recording and there, the left figure is BEFORE and the right figure is AFTER (sorry this is all very confusing)

Overall this looks sensible, but I am curious how it interacts with the tab indexing/accessibility.

Generally speaking screen readers will ignore everything that is display: none;

It'd also be good to get a comment somewhere in the code to say we deliberately keep the sublists rendered so we don't accidentally optimize it back out 😅

Will do 👍

[turt2live]

otherwise lgtm