Apply strictNullChecks to src/LegacyCallHandler.tsx #10690
Conversation
artcodespace
added
the
T-Task
src/LegacyCallHandler.tsx
Outdated
| function isNonNull<T>(thing: T | null): thing is T { | ||
| return thing !== null; | ||
| } | ||
|
|
There was a problem hiding this comment.
This will probably need to move somewhere else - going to ask about whether we have a place for typeguards
| @@ -581,7 +583,9 @@ export default class LegacyCallHandler extends EventEmitter { | |||
| call.on(CallEvent.Hangup, () => { | |||
| if (!this.matchesCallForThisRoom(call)) return; | |||
|
|
|||
| this.removeCallForRoom(mappedRoomId); | |||
| if (isNotNull(mappedRoomId)) { | |||
There was a problem hiding this comment.
Can we just do ?
if (mappedRoomId) {
this.removeCallForRoom(mappedRoomId)
}| @@ -597,8 +601,10 @@ export default class LegacyCallHandler extends EventEmitter { | |||
| this.pause(AudioID.Ringback); | |||
| } | |||
|
|
|||
| this.removeCallForRoom(mappedRoomId); | |||
| this.addCallForRoom(mappedRoomId, newCall); | |||
| if (isNotNull(mappedRoomId)) { | |||
| @@ -691,7 +700,10 @@ export default class LegacyCallHandler extends EventEmitter { | |||
| } | |||
| case CallState.Ended: { | |||
| const hangupReason = call.hangupReason; | |||
| this.removeCallForRoom(mappedRoomId); | |||
| if (isNotNull(mappedRoomId)) { | |||
There was a problem hiding this comment.
Same ^^ if (mappedRoomId)
| @@ -723,7 +735,9 @@ export default class LegacyCallHandler extends EventEmitter { | |||
| this.play(AudioID.CallEnd); | |||
| } | |||
|
|
|||
| this.logCallStats(call, mappedRoomId); | |||
| if (isNotNull(mappedRoomId)) { | |||
| room_id: roomId, | ||
| metricsTrigger: "WebDialPad", | ||
| }); | ||
| if (isNotNull(roomId)) { |
src/LegacyCallHandler.tsx
Outdated
| joining: false, | ||
| metricsTrigger: undefined, // other | ||
| }); | ||
| if (isNotNull(dmRoomId)) { |
| @@ -1175,15 +1192,18 @@ export default class LegacyCallHandler extends EventEmitter { | |||
| const widget = WidgetStore.instance.getApps(roomId).find((app) => WidgetType.JITSI.matches(app.type)); | |||
| if (widget) { | |||
| // If there already is a Jitsi widget, pin it | |||
| WidgetLayoutStore.instance.moveToContainer(client.getRoom(roomId), widget, Container.Top); | |||
| const room = client.getRoom(roomId); | |||
| if (isNotNull(room)) { | |||
It is simpler, but we recently discussed not using implicit coercion to Booleans. Let me see if I can find the meeting notes where we discussed it and what the outcome was. Notes were from 6 Apr (in the Element Web Weekly notes). My reading of the meeting notes is we should prefer explicit comparison over coercion, which I think is what I have done here. In fact @justjanne some of the comments in the notes were attributed to you, interested to hear what you think too. |
|
@alunturner It's important to be careful as falsy isn't always what we'd want it to be. In fact, I actually saw another case of a falsy value (first enum member) being treated as if it was false when it shouldn't have been just this week: matrix-org/matrix-js-sdk#3248 (comment) So I'd keep the isNotNull helper. |
|
@florianduros 👋 I'm back after a few days away and trying to get some stalled work moving. What do you think is the way ahead here? I think we should stick with the |
|
@alunturner okay, seems fine to me ! |
Closes element-hq/element-web#25087
Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.