Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply more strict typescript around the codebase #2778

Merged
merged 35 commits into from
Oct 21, 2022
Merged

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Oct 19, 2022

Requires matrix-org/matrix-react-sdk#9474


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

@t3chguy t3chguy added the T-Task Tasks for the team like planning label Oct 19, 2022
@t3chguy
Copy link
Member Author

t3chguy commented Oct 20, 2022

Increases overall coverage from 67.6% to 68% - infeasible to hit 80% mark with this typing fix.

@t3chguy t3chguy marked this pull request as ready for review October 20, 2022 14:42
@t3chguy t3chguy requested a review from a team as a code owner October 20, 2022 14:42
Copy link
Contributor

@kerryarchibald kerryarchibald left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, left a couple small questions.

@@ -109,10 +109,10 @@ export class UnstableValue<S extends string, U extends string> extends Namespace
}

public get name(): U {
return this.unstable;
return this.unstable!;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be U | undefined return type rather than non-null assertion

Copy link
Member Author

Choose a reason for hiding this comment

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

The constructor says

// Stable is optional, but one of the two parameters is required, hence the weird-looking types.
// Goal is to to have developers explicitly say there is no stable value (if applicable).

So we can assert it will be defined given the stable wasn't. Will try express it with an overload

src/NamespacedValue.ts Show resolved Hide resolved
@@ -685,14 +686,6 @@ export class MatrixEvent extends TypedEventEmitter<MatrixEventEmittedEvents, Mat
* attempt is completed.
*/
public async attemptDecryption(crypto: Crypto, options: IDecryptOptions = {}): Promise<void> {
// For backwards compatibility purposes
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this qualify as a breaking change even though the signature is unchanged?

Copy link
Member Author

Choose a reason for hiding this comment

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

The type didn't allow for the backwards compatibility so it got partially removed at some point in the past

@t3chguy t3chguy merged commit 867a0ca into develop Oct 21, 2022
@t3chguy t3chguy deleted the t3chguy/tsc-strict2 branch October 21, 2022 10:44
@dbkr dbkr mentioned this pull request Oct 24, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants