Skip to content

Conversation

@nvs119
Copy link
Contributor

@nvs119 nvs119 commented Feb 20, 2025

Description

Updating sandbox to follow websocket protocol changes in MMS

Checklist

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@nvs119 nvs119 requested a review from Anemy February 20, 2025 18:37
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

lgtm, the check is failing though so we should fix that first

} else if (typeByte === MESSAGE_TYPE.BINARY) {
return message.subarray(1);
} else {
console.error('message does not have valid type byte "%s":', message);
Copy link
Member

@Anemy Anemy Feb 20, 2025

Choose a reason for hiding this comment

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

The check script for this package is failing because of the no-console lint rule:

error  Unexpected console statement  no-console

To try it locally you can run npm run check in the packages/compass-web folder.
If these are useful we could add eslint ignores. Do we want to know when this happens though? Would it be worth doing a more prominent error here so folks run into it?

Also just noticed is that : at the end of the message extra?

There's another of these in ws-proxy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think it's important enough to leave since it would mean the protocol really screwed up OR some very unexpected type of message is coming through the pipeline. I'll add // eslint-disable-next-line no-console. And thanks for the : catch! Removing

@nvs119 nvs119 merged commit 16724bf into main Feb 20, 2025
17 of 18 checks passed
@nvs119 nvs119 deleted the COMPASS-8976_2 branch February 20, 2025 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants