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

Make more of the codebase conform to strict types #10857

Merged
merged 7 commits into from
May 16, 2023
Merged

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented May 11, 2023

For element-hq/element-web#25088

Split into meaningful commits to aid review


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 May 11, 2023
@t3chguy t3chguy self-assigned this May 11, 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.

Looks good to me, just one q about error type guards

@@ -580,13 +580,13 @@ export default class ContentMessages {
} catch (error) {
// 413: File was too big or upset the server in some way:
// clear the media size limit so we fetch it again next time we try to upload
if (error?.httpStatus === 413) {
if (error instanceof HTTPError && error.httpStatus === 413) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I just love typeguards too much, but I feel that a handful of isHTTPError()/isMatrixError() typeguards could be helpful due to how much they'd get used. What do you think?

Copy link
Member Author

@t3chguy t3chguy May 16, 2023

Choose a reason for hiding this comment

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

I agree for interfaces, but i prefer the built-in instanceof personally where possible, a typeguard leaves ambiguity on the table in the case of inheritence. If I pass a MatrixError to isHTTPError will it return true or false? I'd need to consult the docs, whereas I know instanceof will because MatrixError extends HTTPError

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that's an interesting point. I think I generally use the typeguards for simpler cases and hadn't considered inheritance.

@t3chguy t3chguy merged commit 6a3f59c into develop May 16, 2023
@t3chguy t3chguy deleted the t3chguy/types668 branch May 16, 2023 13:25
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