Typescript strict null checks pt1 #6583
Conversation
Palid
force-pushed
the
dx/typescript-strict-null/pt1
branch
from
b51cf4f to
84cad17
Compare
| private dispatcherRef: string | null = null; | ||
| private supportsPstnProtocol = false; | ||
| private pstnSupportPrefixed = false; // True if the server only support the prefixed pstn protocol | ||
| private supportsSipNativeVirtual = false; // im.vector.protocol.sip_virtual and im.vector.protocol.sip_native |
There was a problem hiding this comment.
These started off as null because we only have a definitive yes/no answer after we've checked with the server, so we're losing this info.
There was a problem hiding this comment.
The question is: do we need this info, or just falling back by default to it being turned off isn't enough?
| const timeUntilTurnCresExpire = turnServersExpiry - Date.now(); | ||
| console.log("Current turn creds expire in " + timeUntilTurnCresExpire + " ms"); | ||
| } else { | ||
| if (process.env.NODE_ENV === 'development') { |
There was a problem hiding this comment.
This isn't really growing on me. I'd rather get loglevel in to do things like this, or wrap it up in an assert() that's a nop in production mode or something.
Moreover, in this case, I forget how this works exactly, but what happens when the client can't get turn creds (in which case we should be attempting the call anyway).
There was a problem hiding this comment.
In some cases it just looked like we aren't properly checking for failures and I honestly have no idea what should happen here. This seemed like the best way to do it without breaking current experience, but it doesn't sound correct.
| @@ -848,6 +853,8 @@ export default class CallHandler extends EventEmitter { | |||
| const call = payload.call as MatrixCall; | |||
|
|
|||
| const mappedRoomId = CallHandler.sharedInstance().roomIdForCall(call); | |||
| // Leave early if can't figure out room id. | |||
| if (!mappedRoomId) return; | |||
There was a problem hiding this comment.
In what circumstances does this happen?
| @@ -901,6 +910,7 @@ export default class CallHandler extends EventEmitter { | |||
| } | |||
|
|
|||
| const call = this.calls.get(payload.room_id); | |||
| if (!call) return; | |||
| // setting invalid content removes it | ||
| WidgetUtils.setRoomWidget(roomId, w.id); | ||
| }); | ||
| if (roomInfo) { |
| <div className="mx_CallEvent_sender"> | ||
| { sender } | ||
| <div style={{backgroundColor: 'red'}}> | ||
| this actually works. |
There was a problem hiding this comment.
I'm assuming this was something else :p
There was a problem hiding this comment.
Oops, a leftover from hmr testing. Stash fail. 😅
| @@ -221,7 +221,7 @@ export function replaceByRegexes(text: string, mapping: Tags): React.ReactNode; | |||
| export function replaceByRegexes(text: string, mapping: IVariables | Tags): string | React.ReactNode { | |||
| // We initially store our output as an array of strings and objects (e.g. React components). | |||
| // This will then be converted to a string or a <span> at the end | |||
| const output = [text]; | |||
| const output: Array<string | React.ReactNode> = [text]; | |||
There was a problem hiding this comment.
isn't ReactNode supposed to be either a react element or a string?
| @@ -438,6 +438,7 @@ export default class SettingsStore { | |||
| settingName = setting.invertedSettingName; | |||
| value = !value; | |||
| } | |||
| if (!roomId) return; | |||
There was a problem hiding this comment.
It should, but half of the usages have roomId as optional, so we need to change the API here. The question is: can we safely rework this API without deprecation, or do we need to properly deprecate it with old params?
|
It seems that @SimonBrandner wants to take over this one, so please do! 😄 |
|
I don't have the time to handle this atm |
MadLittleMods
added
Z-Community-PR
|
Thanks for this but it has rotted beyond usability, some of these sites have already been fixed, rest will be addressed as part of element-hq/element-web#21967 |
Removed some unused variables (mostly private ones) and changed the types accordingly to
strictNullCheckin tsconfig.Please don't mind failing eslint so far, we need to figure this out slightly better. Maybe change APIs in case of the
SettingsStoretoo (make new API and deprecate current one?)Related to element-hq/element-web#21967
This change is marked as an internal change (Task), so will not be included in the changelog.