Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Conform more of the codebase to strictNullChecks #10672

Merged
merged 10 commits into from
Apr 21, 2023
Merged

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Apr 20, 2023

For element-hq/element-web#21967


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

@t3chguy t3chguy added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Apr 20, 2023
@t3chguy t3chguy marked this pull request as ready for review April 20, 2023 10:18
@t3chguy t3chguy requested a review from a team as a code owner April 20, 2023 10:18
@t3chguy t3chguy requested review from dbkr and artcodespace April 20, 2023 10:18
@t3chguy t3chguy self-assigned this Apr 20, 2023
Copy link
Contributor

@artcodespace artcodespace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. One small comment and also a question about Searching.ts - are we expecting this to be completely clear of TS issues? I see a few TS errors there.

@@ -96,13 +97,14 @@ interface IMemberScore {
numRooms: number;
}

export function buildMemberScores(cli: MatrixClient): { [key: string]: IMemberScore | undefined } {
export function buildMemberScores(cli: MatrixClient): { [key: string]: IMemberScore } {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor point - not sure if we prefer <Record<string, ...> for cases like this (also valid elsewhere in this file)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a case where the key string is uselessly key I agree, but IMO this form is better as the key can be given a helpful name, I'll update it to userId for self-documenting

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd never considered that, makes sense

@t3chguy
Copy link
Member Author

t3chguy commented Apr 21, 2023

are we expecting this to be completely clear of TS issues? I see a few TS errors there.

Not yet, not the best way to do things but still helps the overall goal

@t3chguy t3chguy merged commit be5928c into develop Apr 21, 2023
@t3chguy t3chguy deleted the t3chguy/tsc-strict112 branch April 21, 2023 10:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants