Skip to content

Major ToolSocket Refactor#7

Merged
dangond-ptc merged 22 commits intomainfrom
refactor
Apr 11, 2024
Merged

Major ToolSocket Refactor#7
dangond-ptc merged 22 commits intomainfrom
refactor

Conversation

@dangond-ptc
Copy link
Copy Markdown
Contributor

Summary

  • Confusing class hierarchy and parameters now flattened. No more toolSocket.socket.socket. Ever.
  • Many objects now get their own classes with proper documentation. No more cbSub, objBin, or packageData.
  • Major classes now get their own files. No more bloated index.js.
  • Properties of ToolSocketMessage have getters/setters with legible names. No more objBin.o or objBin.f.
  • JSDoc comments are present throughout with proper type annotations. No more wondering if 'binaryBuffer' is supposed to be an Array, a Uint8Array, or an ArrayBuffer.
  • Schemas are now built-up from individual SchemaValidators, which cleans up validation logic.
    • Types here help prevent users from setting maximum for a validator when they meant max-length.
  • New Jest unit tests for new classes and integration tests between the client and server.
  • Improved README now more concise and clear.
  • Bumped package.json version to 2.0.0.

New Classes

ToolSocketMessage - The underlying WebSocket message format.
ToolSocketResponse - An object that can be used to send a response to an incoming message.
MessageBundle - An object that stores the message body (ToolSocketMessage) and associated binary data if present.
BinaryBuffer - A helper object used for processing incoming binary data that has been split across multiple messages.
Schema - An object that offers utilities for parsing URLs/routes and validating object data formats.
SchemaValidator - An object used in the construction of Schemas that defines how URL/route/object properties will be validated.

@dangond-ptc
Copy link
Copy Markdown
Contributor Author

Question: Do we need to support earlier versions of Node? As far as I can tell, no one uses toolsocket except for us.

@dangond-ptc dangond-ptc marked this pull request as ready for review April 9, 2024 14:21
Comment thread README.md Outdated
// Method-style request handling
socket.on('post', (route, body, res, binaryData) => {
if(route === "/") {
console.log(body) // "hello"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: missing semicolon

Comment thread README.md Outdated

// Event-style request handling (Cannot send responses)
socket.on('/', (body, binary) => {
console.log(body) // "hello"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ibid

Comment thread src/MessageBundle.js
const messageBuffer = encoder.encode(JSON.stringify(this.message));
const messageLengthBuffer = intToByte(messageBuffer.length);
const binaryDataBuffer = this.binaryData ? new Uint8Array(this.binaryData) : null;
// Apologies for the terrible naming in this block, but this keeps the following line cleaner by hiding the ternary operator
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you have NOTHING to apologize for!!!

Comment thread src/ToolSocket.js
}

if (!MESSAGE_BUNDLE_SCHEMA.validate(messageBundle.message)) {
console.warn('message schema validation failed, dropping', messageBundle.message, MESSAGE_BUNDLE_SCHEMA.failedValidator);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

very nice improvement in transparency!

Comment thread src/ToolSocketMessage.js
* frameCount - Number of binary buffers that will be sent following this message
*/
constructor(origin, network, method, route, body, id) {
// These parameters are impossible to read in code, use the getters and setters
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤣

@hobinjk-ptc
Copy link
Copy Markdown
Contributor

oh and feel free to replace the test node version matrix with [16, 18, 20]

Copy link
Copy Markdown

@benptc benptc left a comment

Choose a reason for hiding this comment

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

I read through everything line by line and I have no critiques to give – fantastic work! I never really understood how toolsocket works, but now it's easily readable 🥇

@dangond-ptc dangond-ptc merged commit bf8e2be into main Apr 11, 2024
@dangond-ptc dangond-ptc deleted the refactor branch April 11, 2024 18:06
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