Skip to content
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

add typescript dependency, check jsdoc types #38

Merged
merged 2 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const DEFAULT_PORT = 4382;
const log = (...args) => console.error(new Date().toISOString(), ...args);

module.exports = async process => {
const argv = yargs(hideBin(process.argv))
const argv = await yargs(hideBin(process.argv))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yargs can return a promise. So I added an await here in case that happens.

.option('port', {
coerce(string) {
if (!/^(0|[1-9][0-9]*)$/.test(string)) {
Expand Down
53 changes: 35 additions & 18 deletions lib/create-command-server.js
Original file line number Diff line number Diff line change
@@ -1,21 +1,9 @@
'use strict';

const { WebSocketServer } = require('ws');
const interactionModule = require('./modules/interaction');
const sessionModule = require('./modules/session');

const broadcast = (server, message) => {
const packedMessage = JSON.stringify(message);
server.clients.forEach(websocket => {
if (websocket.sessionId) {
websocket.send(packedMessage, error => {
if (error) {
server.emit('error', `error sending message: ${error}`);
}
});
}
});
};

const methodHandlers = {
...interactionModule,
...sessionModule,
Expand Down Expand Up @@ -70,27 +58,56 @@ const onConnection = websocket => {
});
};

class CommandServer extends WebSocketServer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Beautiful

/**
* Broadcast message to all clients.
* @param message
*/
broadcast(message) {
const packedMessage = JSON.stringify(message);
/** @type {Set<WebSocketWithData>} */ (this.clients).forEach(websocket => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of makes me feel like we should be using a WeakMap to associate the session ID. Not in this patch, of course.

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 that sounds like a good idea. Adding post-hoc properties like sessionId makes it hard to track with type analysis where it is assigned or used.

if (websocket.sessionId) {
websocket.send(packedMessage, error => {
if (error) {
this.emit('error', `error sending message: ${error}`);
}
});
}
});
}
}

/**
* Create a server which communicates with external clients using the WebSocket
* protocol described in the project README.md file.
*
* @param {number} port - the port on which the server should listen for new
* connections
*
* @returns {Promise<EventEmitter>} an eventual value which is fulfilled when
* the server has successfully bound to the
* requested port
* @returns {Promise<CommandServer>} an eventual value which is fulfilled when
* the server has successfully bound to the
* requested port
*/
module.exports = async function createWebSocketServer(port) {
const server = new WebSocketServer({
const server = new CommandServer({
clientTracking: true,
path: '/session',
port,
});
await new Promise(resolve => server.once('listening', resolve));

server.broadcast = broadcast.bind(null, server);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a key not defined on the type (in this case WebSocketServer) produces an error. So I extended WebSocketServer as CommandServer and added the method to it.

server.on('connection', onConnection);

return server;
};

/** @typedef {import("ws").WebSocket} WebSocket */

/**
* @typedef WebSocketData
* @property {string} [sessionId]
*/

/**
* @typedef {WebSocket & WebSocketData} WebSocketWithData
*/
2 changes: 2 additions & 0 deletions lib/create-voice-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,5 @@ module.exports = function createVoiceServer(handle) {
server.on('connection', onConnection.bind(null, server));
});
};

/** @typedef {import("events").EventEmitter} EventEmitter */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@types/node doesn't add EventEmitter globally. Alternatively to this we could const {EventEmitter} = require('events') at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that alternative leaves a dangling import. Since EventEmitter isn't used in the code, only in comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for offering alternatives, but I agree with the choice you've made here. I prefer declaring types at the top of the file, though.

Copy link
Contributor Author

@mzgoddard mzgoddard Feb 14, 2024

Choose a reason for hiding this comment

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

I prefer declaring types at the top as well. Applying that how should we manage what the comment modifies? Like in this file if it is put after the imports, it would be documenting onConnection.

I see two solutions for that while keeping the type in the file:

  1. Add a comment documenting a line of code between typedef and none code comments.
  2. Put comments like these at the end of the file.

To make the PR smaller and easier to review I chose the second.

I'll put these typedefs to the top and add comments like stated in 1. Does that sound good? Do you have something else in mind?

Copy link
Contributor Author

@mzgoddard mzgoddard Feb 14, 2024

Choose a reason for hiding this comment

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

Hrm. Are you asking for a change to be made here? On second review of your comment I'm not sure if you are commenting on it or asking for a change.

If so, I'd be happy to do the stroke-out section I stated:

I'll put these typedefs to the top and add comments like stated in 1. Does that sound good? Do you have something else in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was asking for a change, although I thought @typedef always works as a standalone declaration. If we have to insert some weird comment as a delimiter, then I guess that's still slightly better than having types squirreled away at the bottom. Is a delimiter like that really necessary, though? On my machine, the compiler seems fine with this change:

diff --git a/lib/create-voice-server.js b/lib/create-voice-server.js
index c1386c4..e14f78d 100644
--- a/lib/create-voice-server.js
+++ b/lib/create-voice-server.js
@@ -2,6 +2,8 @@
 
 const net = require('net');
 
+/** @typedef {import("events").EventEmitter} EventEmitter */
+
 const onConnection = (server, socket) => {
   let emitted = '';
   socket.on('data', buffer => (emitted += buffer.toString()));
@@ -46,4 +48,3 @@ module.exports = function createVoiceServer(handle) {
   });
 };
 
-/** @typedef {import("events").EventEmitter} EventEmitter */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could also be wrong and remembering something from an older version of typescript? I retract that stuff I said.

(It's pretty nice to see I'm wrong on this. I super thought this was a thing.)

65 changes: 64 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 9 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
"postinstall": "Release\\MakeVoice.exe",
"postuninstall": "Release\\MakeVoice.exe /u",
"prettier": "prettier --write lib test",
"test": "prettier --check lib test && npm run test-unit",
"test-unit": "mocha --ui tdd test"
"test": "prettier --check lib test && npm run test-types && npm run test-unit",
"test-unit": "mocha --ui tdd test",
"test-types": "tsc -p tsconfig.json"
},
"files": [
"lib",
Expand All @@ -22,8 +23,13 @@
"author": "",
"license": "MIT",
"devDependencies": {
"@types/mocha": "^10.0.6",
"@types/node": "^20.11.17",
"@types/ws": "^8.5.10",
"@types/yargs": "^17.0.32",
"mocha": "^9.1.2",
"prettier": "^3.2.4"
"prettier": "^3.2.4",
"typescript": "^5.3.3"
},
"dependencies": {
"robotjs": "^0.6.0",
Expand Down
7 changes: 7 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"compilerOptions": {
"checkJs": true,
"noEmit": true
},
"include": ["lib", "test"]
}