message_events: Convert module to typescript. #32492
Conversation
evykassirer
added
the
area: typescript migration
evykassirer
force-pushed
the
message-events-typescript
branch
from
8ee4c56 to
5cd85f9
Compare
|
Made the changes Anders suggested, though the PR description is still accurate. |
evykassirer
force-pushed
the
message-events-typescript
branch
from
5cd85f9 to
f0c9ac7
Compare
evykassirer
force-pushed
the
message-events-typescript
branch
from
f0c9ac7 to
b646054
Compare
web/src/message_events.ts
Outdated
| function filter_has_term_type(filter: Filter | undefined, term_type: string): boolean { | ||
| return ( | ||
| filter !== undefined && |
There was a problem hiding this comment.
Accepting Filter | undefined here has no advantage; every caller either passes a defined Filter or has to later check for undefined anyway.
| function filter_has_term_type(filter: Filter | undefined, term_type: string): boolean { | |
| return ( | |
| filter !== undefined && | |
| function filter_has_term_type(filter: Filter, term_type: string): boolean { | |
| return ( |
| assert(first_message !== undefined); | ||
| const first_id = first_message.id; | ||
| const last_message = msg_list.last(); | ||
| assert(last_message !== undefined); |
There was a problem hiding this comment.
Do we know these aren’t undefined?
There was a problem hiding this comment.
If they had been, the previously existing code would have given an error for calling .id on undefined
There was a problem hiding this comment.
Yeah I'm not sure the previous code is correct here; @amanagr can investigate as a follow-up.
evykassirer
force-pushed
the
message-events-typescript
branch
from
b646054 to
b8232a5
Compare
andersk
added
the
integration review
Accepting an undefined Filter has no advantage; every caller either passes a defined Filter or has to later check for undefined anyway.
evykassirer
force-pushed
the
message-events-typescript
branch
from
b8232a5 to
b5d14a9
Compare
| prev_stream?: number; | ||
| topic?: string; | ||
| prev_topic?: string; | ||
| } = { |
There was a problem hiding this comment.
Probably a good follow-up would be to refine this type to be a union of different types of edit history event types used elsewhere in the codebase. EditHistoryEntry is fairly different. Might be appropriate to just leave an issue for this if it seems like work.
|
Merged, thanks @evykassirer! Pretty excited to have this one merged, since it's one of the last handful of big files, and one of the more logically complex. |
4 participants
There are a handful of things I made assumptions about that I need to check, one of which is related to a failing test, but opening this as a draft for now in case anyone wants to take a first pass review on it.