-
Notifications
You must be signed in to change notification settings - Fork 8.3k
Define types for WebSocket messages and migrate WebSocket actions to TS #34603
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
Conversation
|
This is going to conflict with the burn-on-read feature branch which adds some new WS event types (#34601) and new WS event handling |
|
||||||
| Test | Retries |
|---|---|
| Postgres (Results) | |
| com/mattermost/mattermost/server/v8/channels/app.TestAddUserToChannelCreatesChannelMemberHistoryRecord | 1 |
| WebsocketEventReactionRemoved WebsocketEventType = "reaction_removed" | ||
| WebsocketEventResponse WebsocketEventType = "response" | ||
| WebsocketEventEmojiAdded WebsocketEventType = "emoji_added" | ||
| WebsocketEventChannelViewed WebsocketEventType = "channel_viewed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed a few events that aren't used any more. This one was replaced by multiple_channels_viewed, and I couldn't find any references to that Cloud one
|
|
||
| const msgPropsPost: Post = JSON.parse(msgProps.post); | ||
| const attachments = isMessageAttachmentArray(msgPropsPost?.props?.attachments) ? msgPropsPost.props.attachments : []; | ||
| const attachments = isMessageAttachmentArray(post.props?.attachments) ? post.props.attachments : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already parsed the post before, so I don't know why we were parsing it again
| dispatch(completePostReceive(post, websocketMessageProps, myChannelMemberDoesntExist)); | ||
|
|
||
| if (msg && msg.data) { | ||
| if (msg && msg.data && 'channel_type' in msg.data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to differentiate between posted and ephemeral_post events
| }; | ||
|
|
||
| /** | ||
| * @deprecated Use WebSocketEvents from @mattermost/client instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would've removed this entirely, but there's a few extra WS events left over which come from the Apps plugin which I didn't want to move into the new code
| // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. | ||
| // See LICENSE.txt for license information. | ||
|
|
||
| import type * as WebSocketMessages from './websocket_messages'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of just re-exporting these types like we do below, I decided to do this slightly differently to wrap all of these message types in sort of a namespace. That means you have to use these types like WebSocketMessages.Posted instead of something like WebSocketPostedMessage, but it also means that we don't need to import 80+ individual message types into websocket_actions.ts. If we went back to writing more imports like import * as Utils from ..., that would be less of a problem, but I think this is a reasonable middleground
| import type {WebSocketEvents} from './websocket_events'; | ||
| import type * as Messages from './websocket_messages'; | ||
|
|
||
| export type WebSocketMessage = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, you wouldn't need to define this union type, the constant, and the actual type separately, but without looking into generating TS types (which I've briefly tried multiple times), we're stuck with a bit of boilerplate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be split up, but I felt like that would make the boilerplate feeling worse
|
||||||||||
| Test | Retries |
|---|---|
| Postgres (Results) | |
| com/mattermost/mattermost/server/v8/channels/api4.TestCreatePost/should_prevent_creating_post_with_files_when_user_lacks_upload_file_permission_in_target_channel | 1 |
| com/mattermost/mattermost/server/v8/channels/api4.TestCreatePost/should_allow_creating_post_with_files_when_user_has_upload_file_permission | 1 |
| com/mattermost/mattermost/server/v8/channels/api4.TestCreatePost | 1 |
|
E2E test run is starting for commit
You can also look at the E2E test's Workflow Run URL (run ID |
|
E2E test has completed for commit
The run summary artifacts are available in the corresponding Workflow Run. |
larkox
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
| display_name: msgProps.channel_display_name, | ||
| type: msgProps.channel_type, | ||
| }; | ||
| } as Channel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often do we go through this branch? How safe is to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's no less safe than what we're currently doing. I think you might actually be the one who added this fallback previously 😛
Looking through the code below though, we only use the 4 fields specified here in every case except for when we pass it to a plugin hook (registerDesktopNotificationHook) where there's an as any. It's totally safe then except for the plugin case which was previously a potential issue anyway.
Thinking about the code as a whole though, we currently assume that the user has all of their channels loaded, so this branch should never actually trigger
|
E2E test run is starting for commit
You can also look at the E2E test's Workflow Run URL (run ID |
|
E2E test has completed for commit
The run summary artifacts are available in the corresponding Workflow Run. |
|
E2E test run is starting for commit
You can also look at the E2E test's Workflow Run URL (run ID |
|
E2E test has completed for commit
The run summary artifacts are available in the corresponding Workflow Run. |
harshilsharma63
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved ✅
|
@yasserfaraazkhan This is another PR that shouldn't have any effect on users, and any functional changes are hopefully just for handling edge cases which may or may not be possible. I'm not sure if you want to smoke test this or if the E2E tests passing is sufficient |
|
Mattermost Spinwick PR #34603 🎉 Test server created! Access here: https://mattermost-pr-34603-aba57.test.mattermost.cloud Installation ID: Credentials: Posted securely in this Mattermost channel - Look for PR #34603 |
|
Test server destroyed |
Summary
Typing the WebSocket code means we're less likely to break stuff in the future, and it provides an interface for people working with WebSocket code so that they know what to expect when handling WebSocket events. We could also generate some docs from these type defintions now as well.
For developers adding a new WS event, they will now:
utils/constantsormattermost-redux/constants/websocket, add that toplatform/client/src/websocket_events.ts. Now, there's only one place for that to go instead of having multiple confliciting places for that.platform/client/src/websocket_message.tsfor the message body, and then add that type to the union type inplatform/client/src/websocket_messages.tsso that the type inference for the WS messages works.websocket_actionsas before. The type inference should all give you a properly typed event and everythingTicket Link
Release Note