Move subscriptions out of RoomView constructor #9973

clarkf · 2023-01-23T21:20:30Z

Submitting this separately from the other instances since it's... a bit hairy. Best reviewed one commit at a time, I reckon.

My objective is to move the subscription registration out of RoomView's constructor. React doesn't guarantee that componentWillUmount (i.e. 'the cleanup function') will be called unless componentDidMount was called. This means all those subscriptions were possibly not being cleaned up, i.e. leaking.

In order to get there, I had to get RoomView.ts (at least) to pass the CI checks, namely the --noImplicitAny and --strict checks. Since #9940 is a thing, and in order to minimize conflicts, I pulled those changes pretty much directly. I had to touch a few signatures in other files, so CI will still probably complain.

This may fix element-hq/element-web#10826

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

Fix a couple issues with the RoomView tests: * Many tests were rendering with no room membership, resulting in just the "do you want to join?" prompt. * Some tests were erroring, which was caught by error boundaries. * Some tests were still using enzyme. Those have been migrated to react-testing-library. Signed-off-by: Clark Fischer <[email protected]>

Signed-off-by: Clark Fischer <[email protected]>

Components are not 'cleaned up' (i.e. `componentWillUnmount` called) unless `componentDidMount` was called. React doesn't expect to have to simulate an unmount if the component was never mounted to begin with. In the case of RoomView, that means that the subscriptions created in the constructor are never removed, i.e. a leak. This moves the subscriptions to the `componentDidMount` method. See element-hq/element-web#24236 (comment) Signed-off-by: Clark Fischer <[email protected]>

weeman1337 · 2023-01-30T07:40:48Z

Thanks for you PR @clarkf . Do you think it is possible to raise the test coverage for changed code to at least 80 %?

weeman1337

I am stopping my review somewhere in the middle. Moving the subscriptions out of the constructor makes sense 👍 But this PR also targets fixing all the type issues in RoomView.

I would suggest to split this up: One PR for the type fixes and another to move out the subscriptions.

For the type fixes: In many places just adding a ! to silence the linter is probably not what we should do. Instead the migration may contain more checks, if something is set and logging.

src/components/structures/RoomView.tsx

weeman1337 · 2023-01-30T07:57:41Z

src/components/structures/RoomView.tsx

@@ -587,18 +540,18 @@ export class RoomView extends React.Component<IRoomProps, IRoomState> {
            // The rest will be lost for now, until the aggregation API on the server
            // becomes available to fetch a whole thread
            if (!initialEvent) {
-                initialEvent = await fetchInitialEvent(this.context.client, roomId, initialEventId);
+                initialEvent = (await fetchInitialEvent(client, roomId!, initialEventId)) || undefined;


Does a RoomView work without a room Id? We should probably test if the room Id is set somewhere before instead of using !.

roomId has to be nullish (as does room). It's a state variable, so it can't be known until the component mounts and the RoomViewStore is checked. Immediately after the component mounts, roomId is (basically) guaranteed to be defined, as long as the entire app is in a valid state. I agree that it's messy, but I think the scope of this PR is already an issue without more sweeping changes.

There is a test earlier in the method, it's just an ordering artifact I'll clear up.

weeman1337 · 2023-01-30T09:45:09Z

src/components/structures/RoomView.tsx


            const thread = initialEvent?.getThread();
            if (thread && !initialEvent?.isThreadRoot) {
                dis.dispatch<ShowThreadPayload>({
                    action: Action.ShowThread,
-                    rootEvent: thread.rootEvent,
+                    rootEvent: thread.rootEvent!, // isThreadRoot guarantees


Logically yes. Technically it doesn't. Shouldn't initialEvent be the root event?

I'm not familiar enough with the threads API to say. Maybe? There aren't any types here to make this work without some kind of assertion. I'll swap it, I was just trying to change as little as possible while adding context.

weeman1337 · 2023-01-30T09:48:03Z

src/components/structures/RoomView.tsx

            // Get the scroll state for the new room

            // If an event ID wasn't specified, default to the one saved for this room
            // in the scroll state store. Assume initialEventPixelOffset should be set.
            if (!newState.initialEventId) {
-                const roomScrollState = RoomScrollStateStore.getScrollState(newState.roomId);
+                const roomScrollState = RoomScrollStateStore!.getScrollState(newState.roomId);


We should probably use optional chaining here

Suggested change

const roomScrollState = RoomScrollStateStore!.getScrollState(newState.roomId);

const roomScrollState = RoomScrollStateStore?.getScrollState(newState.roomId);

This is kind of a weird one. In reality, RoomScrollStateStore is guaranteed to be defined:

matrix-react-sdk/src/stores/RoomScrollStateStore.ts

Lines 50 to 53 in e9d7232

if (window.mxRoomScrollStateStore === undefined) {

window.mxRoomScrollStateStore = new RoomScrollStateStore();

}

export default window.mxRoomScrollStateStore;

I believe this is a pre-TS artifact, which is why the default export is technically RoomScrollStateStore | undefined. Truthfully, the assertion belongs in that file (since it guarantees its presence).

clarkf · 2023-01-30T17:48:47Z

Regarding the size/scope of this PR, I'm not sure how to proceed. The CI checks won't pass without the strict updates -- I've rewritten this patch set at least 4 times, one of which was an absolutely minimal change -- just the tests and moving the lines from one method to another. The --strict lint didn't pass, nor did the coverage check. RoomView is far too complex to for that. Basically the same issue with the tests. I'll attempt to add some more.

I can separate the first three commits out into their own PR, but I'm not sure how much that would help. There'd just be two PR's.

For the type fixes: In many places just adding a ! to silence the linter is probably not what we should do. Instead the migration may contain more checks, if something is set and logging.

I'll do another pass, looking for opportunities to modify the control flow instead of asserting. Silencing the typechecker was not my intent, in every(?) case, I verified that those things are supposed to be defined. My goal is not to change behavior or rewrite anything.

t3chguy · 2023-08-07T15:06:59Z

@clarkf are you interested in continuing this PR now that the repository has been made to comply with --strict mode?

t3chguy · 2023-10-16T15:27:59Z

Closing due to no response

clarkf requested a review from a team as a code owner January 23, 2023 21:20

clarkf requested review from germain-gg and weeman1337 January 23, 2023 21:20

github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Jan 23, 2023

github-actions bot deployed to Netlify January 23, 2023 21:26 View deployment

clarkf force-pushed the room-view-lifecycle branch from 7d574ef to 55b88c0 Compare January 23, 2023 21:35

github-actions bot deployed to Netlify January 23, 2023 21:39 View deployment

clarkf force-pushed the room-view-lifecycle branch from 55b88c0 to de1b4c6 Compare January 24, 2023 14:23

github-actions bot deployed to Netlify January 24, 2023 14:27 View deployment

clarkf force-pushed the room-view-lifecycle branch from de1b4c6 to 37640df Compare January 26, 2023 22:18

github-actions bot deployed to Netlify January 26, 2023 22:22 View deployment

clarkf force-pushed the room-view-lifecycle branch from 37640df to 85f70ea Compare January 27, 2023 14:02

github-actions bot deployed to Netlify January 27, 2023 14:09 View deployment

clarkf added 4 commits January 27, 2023 11:00

noImplicitAny fixes for RoomView

af139eb

Signed-off-by: Clark Fischer <[email protected]>

Strict fixes for RoomView.tsx

cd588b0

Signed-off-by: Clark Fischer <[email protected]>

clarkf force-pushed the room-view-lifecycle branch from 85f70ea to 24f8f9f Compare January 27, 2023 19:05

github-actions bot deployed to Netlify January 27, 2023 19:09 View deployment

weeman1337 added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Jan 30, 2023

weeman1337 suggested changes Jan 30, 2023

View reviewed changes

germain-gg removed their request for review February 28, 2023 08:13

t3chguy closed this Oct 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move subscriptions out of RoomView constructor #9973

Move subscriptions out of RoomView constructor #9973

clarkf commented Jan 23, 2023 •

edited by github-actions bot

Loading

weeman1337 commented Jan 30, 2023

weeman1337 left a comment

weeman1337 Jan 30, 2023

clarkf Jan 30, 2023

weeman1337 Jan 30, 2023

clarkf Jan 30, 2023

weeman1337 Jan 30, 2023

clarkf Jan 30, 2023

clarkf commented Jan 30, 2023

t3chguy commented Aug 7, 2023

t3chguy commented Oct 16, 2023

	const roomScrollState = RoomScrollStateStore!.getScrollState(newState.roomId);
	const roomScrollState = RoomScrollStateStore?.getScrollState(newState.roomId);

	if (window.mxRoomScrollStateStore === undefined) {
	window.mxRoomScrollStateStore = new RoomScrollStateStore();
	}
	export default window.mxRoomScrollStateStore;

Move subscriptions out of RoomView constructor #9973

Move subscriptions out of RoomView constructor #9973

Conversation

clarkf commented Jan 23, 2023 • edited by github-actions bot Loading

Checklist

weeman1337 commented Jan 30, 2023

weeman1337 left a comment

Choose a reason for hiding this comment

weeman1337 Jan 30, 2023

Choose a reason for hiding this comment

clarkf Jan 30, 2023

Choose a reason for hiding this comment

weeman1337 Jan 30, 2023

Choose a reason for hiding this comment

clarkf Jan 30, 2023

Choose a reason for hiding this comment

weeman1337 Jan 30, 2023

Choose a reason for hiding this comment

clarkf Jan 30, 2023

Choose a reason for hiding this comment

clarkf commented Jan 30, 2023

t3chguy commented Aug 7, 2023

t3chguy commented Oct 16, 2023

clarkf commented Jan 23, 2023 •

edited by github-actions bot

Loading