add typescript dependency, check jsdoc types #38

mzgoddard · 2024-02-13T20:27:30Z

Add typescript dependency
Add test-types npm script
Call npm test-types in npm test script
Add CommandServer class extends WebSocketServer
Add @types for dependencies that need them

- Add typescript dependency - Add test-types npm script - Call npm test-types in npm test script - Add CommandServer class extends WebSocketServer - Add @types for dependencies that need them

mzgoddard · 2024-02-13T20:28:59Z

lib/create-command-server.js

    clientTracking: true,
    path: '/session',
    port,
  });
  await new Promise(resolve => server.once('listening', resolve));

-  server.broadcast = broadcast.bind(null, server);


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.

mzgoddard · 2024-02-13T20:29:54Z

lib/create-voice-server.js

@@ -45,3 +45,5 @@ module.exports = function createVoiceServer(handle) {
    server.on('connection', onConnection.bind(null, server));
  });
 };
+
+/** @typedef {import("events").EventEmitter} EventEmitter */


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

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

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.

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:

Add a comment documenting a line of code between typedef and none code comments.

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?~~

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?

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 */

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.)

mzgoddard · 2024-02-13T20:31:55Z

lib/cli.js

@@ -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))


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

jugglinmike

Thanks, @mzgoddard!

jugglinmike · 2024-02-13T23:25:46Z

lib/create-voice-server.js

@@ -45,3 +45,5 @@ module.exports = function createVoiceServer(handle) {
    server.on('connection', onConnection.bind(null, server));
  });
 };
+
+/** @typedef {import("events").EventEmitter} EventEmitter */


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.

jugglinmike · 2024-02-13T23:27:50Z

lib/create-command-server.js

@@ -70,27 +58,56 @@ const onConnection = websocket => {
  });
 };

+class CommandServer extends WebSocketServer {


jugglinmike · 2024-02-13T23:28:02Z

lib/create-command-server.js

+   */
+  broadcast(message) {
+    const packedMessage = JSON.stringify(message);
+    /** @type {Set<WebSocketWithData>} */ (this.clients).forEach(websocket => {


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

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.

mzgoddard · 2024-02-14T21:51:22Z

I moved @typedef imports and types from bottom of create-command-server and create-voice-server to the top of their respective files before the first non-require statement.

jugglinmike

Right. On.

add typescript dependency, check jsdoc types

56783f8

- Add typescript dependency - Add test-types npm script - Call npm test-types in npm test script - Add CommandServer class extends WebSocketServer - Add @types for dependencies that need them

mzgoddard requested a review from jugglinmike February 13, 2024 20:27

mzgoddard commented Feb 13, 2024

View reviewed changes

jugglinmike requested changes Feb 13, 2024

View reviewed changes

mzgoddard mentioned this pull request Feb 14, 2024

Add macos interaction.pressKeys #37

Merged

fixup! add typescript dependency, check jsdoc types

c3fb1b9

mzgoddard requested a review from jugglinmike February 14, 2024 21:51

jugglinmike approved these changes Feb 14, 2024

View reviewed changes

jugglinmike merged commit 9f3b158 into main Feb 14, 2024

jugglinmike deleted the typecheck branch February 14, 2024 23:21

mzgoddard mentioned this pull request Feb 15, 2024

Formalizing TypeScript dependency w3c/aria-at-automation-harness#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add typescript dependency, check jsdoc types #38

add typescript dependency, check jsdoc types #38

mzgoddard commented Feb 13, 2024 •

edited

Loading

mzgoddard Feb 13, 2024

mzgoddard Feb 13, 2024

mzgoddard Feb 13, 2024

jugglinmike Feb 13, 2024

mzgoddard Feb 14, 2024 •

edited

Loading

mzgoddard Feb 14, 2024 •

edited

Loading

jugglinmike Feb 14, 2024

mzgoddard Feb 14, 2024

mzgoddard Feb 13, 2024

jugglinmike left a comment

jugglinmike Feb 13, 2024

jugglinmike Feb 13, 2024

jugglinmike Feb 13, 2024

mzgoddard Feb 14, 2024

mzgoddard commented Feb 14, 2024

jugglinmike left a comment

add typescript dependency, check jsdoc types #38

add typescript dependency, check jsdoc types #38

Conversation

mzgoddard commented Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzgoddard Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

mzgoddard Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jugglinmike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzgoddard commented Feb 14, 2024

jugglinmike left a comment

Choose a reason for hiding this comment

mzgoddard commented Feb 13, 2024 •

edited

Loading

mzgoddard Feb 14, 2024 •

edited

Loading

mzgoddard Feb 14, 2024 •

edited

Loading