minor jsdoc improvements by jimmywarting · Pull Request #1910 · websockets/ws

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

 * 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

 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

 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

 * 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

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

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

		const mask = Buffer.alloc(4);


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

		const { GUID, kWebSocket } = require('./constants');


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

	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

Uh oh!

Conversation

jimmywarting commented Jul 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmywarting Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmywarting Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented Jul 6, 2021

Uh oh!

jimmywarting commented Jul 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jimmywarting Jul 5, 2021 •

edited

Loading

jimmywarting Jul 5, 2021 •

edited

Loading

lpinca Jul 5, 2021 •

edited

Loading