minor jsdoc improvements #1910

jimmywarting · 2021-07-05T01:25:16Z

I use this settings on by default in VS code

    "javascript.suggest.completeFunctionCalls": true,
    "javascript.suggest.completeJSDocs": true,
    "javascript.suggest.autoImports": true,
    "js/ts.implicitProjectConfig.checkJs": true,

It was showing red lines a bit here and there. With this changes it could figure out more what things where with autocompletion

Could improve some bits even further if i may use private fields, But it requires node v12 and you are testing v8...
could we perhaps drop some older versions and align a bit more with current targeted support versions?

lpinca · 2021-07-05T06:34:58Z

lib/receiver.js

@@ -586,7 +584,7 @@ module.exports = Receiver;
 /**
 * Builds an error object.
 *
- * @param {(Error|RangeError)} ErrorCtor The error constructor
+ * @param {(typeof Error|typeof RangeError)} ErrorCtor The error constructor


Why typeof? It should be the Error or RangeError constructor.

from VS code (TypeScript) perspective it looks at @param {Error} to be a instance of a Error class, when using typeof then it knows that it can be a Class

lpinca · 2021-07-05T06:48:20Z

lib/sender.js

@@ -9,14 +9,17 @@ const { mask: applyMask, toBuffer } = require('./buffer-util');

 const mask = Buffer.alloc(4);

+
+/** @typedef {import('net').Socket} Socket */


This could also be a tls.Socket. Also, no TypeScript-specific syntax please.

i tried tls.Socket, didn't work so well... it could not figure out the correct NodeJS namespace
also typedef is a jsdoc thing

I don't like TypeScript syntax either, very much prefer checkJs + jsDoc combo

if you are running VS-code you can try adding a comment on top and see if you get any benefit from using
// @ts-check at the top of the file

https://www.dandoescode.com/blog/using-typescript-without-typescript/

https://stackoverflow.com/questions/49836644/how-to-import-a-typedef-from-one-file-to-another-in-jsdoc-using-node-js

From the comments:

This is TypeScript-specific syntax. Vote for github.com/jsdoc/jsdoc/issues/1645.

oh, look at that...
anyway another way could be to import a unused variable

var http = require('http') * @param {http.Server} function foo() { }

Best if you have treeshaking support for this case

That's acceptable. However we might need to add an eslint comment for unused variables.

How should we proceed here? Using {net.Socket} seems more popular than {import('net').Socket} even if it doesn't work perfectly (when net is not required).

I would prefer to just improve the existing type with tls.Socket:

@param {(net.Socket|tls.Socket)} socket The connection socket

lpinca · 2021-07-05T06:49:12Z

lib/websocket-server.js

@@ -9,13 +9,14 @@ const WebSocket = require('./websocket');
 const { format, parse } = require('./extension');
 const { GUID, kWebSocket } = require('./constants');

+
+/** @typedef {import('http').Server} Server */


lpinca · 2021-07-05T06:50:13Z

lib/websocket.js

+WebSocket.CONNECTING = 0
+WebSocket.OPEN = 1
+WebSocket.CLOSING = 2
+WebSocket.CLOSED = 3
+


Why this change? It is already done by Object.defineProperty(WebSocket, readyState, descriptor); below.

VS inteligens could not figure out what dynamically assigned properties meant

I prefer to not have changes only required for TypeScript / Visual Studio Code IntelliSense.

Do you have any other jsDoc solution up your sleeve that describes WebSocket to have this properties?

It's weird but something like this should work:

/** * Class representing a WebSocket. * * @property {Number} CONNECTING description */

forgot about that thing

lpinca · 2021-07-05T06:52:17Z

lib/websocket.js

 readyStates.forEach((readyState, i) => {
-  const descriptor = { enumerable: true, value: i };
+  const descriptor = { enumerable: true, value: i, configurable: false, writable: false };


Why this change? false is the default value for configurable and writable.

when i assigned the WebSocket.CLOSING = 2 it probably changed the configurable to true, so i had to override it again - test was failing if i didn't do this

What test? Behavior is tested here

ws/test/websocket.test.js

Lines 102 to 111 in 0ad1f9d

it('is enumerable property of class', () => {

const descriptor = Object.getOwnPropertyDescriptor(WebSocket, state);

assert.deepStrictEqual(descriptor, {

configurable: false,

enumerable: true,

value: readyStates[state],

writable: false

});

});

.

Ah I understand now, this happens due to #1910 (comment). I guess that is another reason to avoid that change.

lpinca · 2021-07-05T08:31:48Z

lib/receiver.js

@@ -586,7 +584,7 @@ module.exports = Receiver;
 /**
 * Builds an error object.
 *
- * @param {(Error|RangeError)} ErrorCtor The error constructor
+ * @param {(typeof Error|typeof RangeError)} ErrorCtor The error constructor


Suggested change

* @param {(typeof Error|typeof RangeError)} ErrorCtor The error constructor

* @param {function(new:Error|RangeError)} ErrorCtor The error constructor

Refs: #1910

lpinca · 2021-07-06T19:23:19Z

@jimmywarting are you ok with #1912?

Refs: #1910

jimmywarting · 2021-07-06T19:36:34Z

closing mine b/c it's superseeded and i'm overwhelmed with other work right now

Refs: #1910

Refs: websockets/ws#1910

jimmywarting added 2 commits July 5, 2021 03:19

minor jsdoc improvements

0204b62

fix failing test

8d531ee

lpinca reviewed Jul 5, 2021

View reviewed changes

lpinca pushed a commit that referenced this pull request Jul 6, 2021

[minor] Improve JSDoc comments

824c295

Refs: #1910

lpinca mentioned this pull request Jul 6, 2021

Improve JSDoc-inferred types #1912

Merged

lpinca pushed a commit that referenced this pull request Jul 6, 2021

[minor] Improve JSDoc comments

222563b

Refs: #1910

lpinca pushed a commit that referenced this pull request Jul 6, 2021

[minor] Improve JSDoc comments

8696a9d

Refs: #1910

jimmywarting closed this Jul 6, 2021

lpinca pushed a commit that referenced this pull request Jul 7, 2021

[minor] Improve JSDoc-inferred types

a8cdb46

Refs: #1910

lpinca pushed a commit that referenced this pull request Jul 7, 2021

[minor] Improve JSDoc-inferred types

a1e0901

Refs: #1910

lpinca added a commit that referenced this pull request Jul 7, 2021

[minor] Improve JSDoc-inferred types (#1912)

ecb9d9e

Refs: #1910

th37rose added a commit to th37rose/websocket_server that referenced this pull request May 24, 2022

[minor] Improve JSDoc-inferred types (#1912)

43c4068

Refs: websockets/ws#1910

jimmywarting deleted the improve-jsdoc branch December 1, 2022 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

minor jsdoc improvements #1910

minor jsdoc improvements #1910

jimmywarting commented Jul 5, 2021

lpinca Jul 5, 2021

jimmywarting Jul 5, 2021

lpinca Jul 5, 2021

jimmywarting Jul 5, 2021 •

edited

jimmywarting Jul 5, 2021

lpinca Jul 5, 2021

jimmywarting Jul 5, 2021 •

edited

lpinca Jul 5, 2021

lpinca Jul 6, 2021

lpinca Jul 5, 2021

lpinca Jul 5, 2021

jimmywarting Jul 5, 2021

lpinca Jul 5, 2021

jimmywarting Jul 5, 2021

lpinca Jul 5, 2021 •

edited

jimmywarting Jul 5, 2021

lpinca Jul 5, 2021

jimmywarting Jul 5, 2021

lpinca Jul 5, 2021

This comment was marked as spam.

lpinca Jul 5, 2021

lpinca Jul 5, 2021

jimmywarting Jul 6, 2021

lpinca commented Jul 6, 2021

jimmywarting commented Jul 6, 2021

		@@ -9,14 +9,17 @@ const { mask: applyMask, toBuffer } = require('./buffer-util');

		const mask = Buffer.alloc(4);


		/** @typedef {import('net').Socket} Socket */

	it('is enumerable property of class', () => {
	const descriptor = Object.getOwnPropertyDescriptor(WebSocket, state);

	assert.deepStrictEqual(descriptor, {
	configurable: false,
	enumerable: true,
	value: readyStates[state],
	writable: false
	});
	});

	* @param {(typeof Error\|typeof RangeError)} ErrorCtor The error constructor
	* @param {function(new:Error\|RangeError)} ErrorCtor The error constructor

minor jsdoc improvements #1910

minor jsdoc improvements #1910

Conversation

jimmywarting commented Jul 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmywarting Jul 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmywarting Jul 5, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Jul 5, 2021 • edited

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

This comment was marked as spam.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca commented Jul 6, 2021

jimmywarting commented Jul 6, 2021

jimmywarting Jul 5, 2021 •

edited

jimmywarting Jul 5, 2021 •

edited

lpinca Jul 5, 2021 •

edited