chore: Remove starMessage client meteor method
#33817
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 5cd1e97 The changes in this PR will be included in the next version bump. This PR includes changesets to release 35 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
| @@ -19,7 +20,8 @@ Meteor.startup(() => { | |||
| const { message = messageArgs(this).msg } = props; | |||
|
|
|||
| try { | |||
| await sdk.call('starMessage', { ...message, starred: true }); | |||
| await sdk.rest.post('/v1/chat.starMessage', { messageId: message._id }); | |||
| starMessage(message, true); | |||
There was a problem hiding this comment.
What will happen if starMessage() fails and throws and error? Shouldn't ['rooms', message.rid, 'starred-messages'] queries be invalidated regardless of this?
There was a problem hiding this comment.
Maybe we should put this query invalidation in a finally block, since it should happen regardless of try or catch block.
There was a problem hiding this comment.
Done as suggested above.
| '@rocket.chat/meteor': patch | ||
| --- | ||
|
|
||
| Remove client side `starMessage` meteor method and replace this meteor method call with rest API. |
There was a problem hiding this comment.
| Remove client side `starMessage` meteor method and replace this meteor method call with rest API. | |
| Removes client side `starMessage` meteor method and replaces this meteor method call with rest API. |
But, I kinda think these changesets are not needed.
Or, more likely replace with something much much more generic, Reduces websocket overhead from web client.
given these are for customers, and not considered to be breaking, they do seem out of place in terms of friendliness and customer facing-ness (if that's a word);
| export const starMessage = (message: IMessage, starred: boolean) => { | ||
| const uid = Meteor.userId(); |
There was a problem hiding this comment.
if this is a new function, shouldn't we be able to just pass the uid instead of depending on meteor?
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
ARCH-1344