[Feature]: Improve Project's JSDoc Types

[ITenthusiasm]

EDIT: This will now only be an improvement to the project's JSDoc types. (See discussion below.)

Is there an existing issue for this?

• I've searched for any related issues and avoided creating a duplicate issue.

Description

From a developer standpoint, it's a little inconvenient that developers have to install ws and @types/ws separately. From a maintainability standpoint, it's more work to manage both the library's code and the library's types separately. Doing so also opens up the door for types to accidentally get out of sync.

I've noticed that this library is so important that even members of the TypeScript team have contributed to the package's type definitions at certain points in time. (Though I don't know how motivated they are to do this in the present.) Having the types and the library code in a central place would provide a great DX.

For clarity, I saw #1623 and #1762. This is not a request to rewrite the project to TypeScript. Instead, it's a request to reuse the JSDoc approach that the library is already leveraging -- just like Rich Harris suggests (and has suggested in many other videos). Some of the existing JSDocs might need a little improvement, but I'm not assuming this would require any kind of overhaul.

I'm happy to contribute to this if the team is open to the idea.

The Approach

If this is something that's of interest, then the approach should be pretty simple. It's pretty much just creating a tsconfig that can check JSDocs. For example:

{
  "compilerOptions": {
    "target": "ES2020",
    "module": "NodeNext",
    "moduleResolution": "NodeNext",
    "strict": true,
    "allowJs": true,
    "checkJs": true,
    "declaration": true,
    "emitDeclarationOnly": true,
    "forceConsistentCasingInFileNames": true
  },
  "include": ["./lib/**/*.js"]
}

Then add a new step to the package release process. (There is no build step.) Basically, this step would tell TypeScript to generate the .d.ts files from the JS files (tsc), and it would include those generated files in the released package. That's pretty much it. Since you won't be managing the .d.ts files yourself, they should probably be added to the .gitignore.

This would not have to be done in one fell swoop. We could try this incrementally by "converting" one file at a time and seeing how it feels. From a brief scan, it seems like most of the "conversions" would be simple (e.g., Number --> number and * --> any/unknown).

ws version

8.x

[ITenthusiasm]

My motivation for this is that I'm currently in the process of rewriting my Medium article on testing ws WebSocket servers with jest. (I recently discovered that there are vastly superior ways to provide helper functions for those kinds of tests.)

Along the way, I found an area where the types could potentially be improved, but I was hesitant to interact with @types/ws because the process at DefinitelyTyped is usually draw out. I thought that direct updates to the library would be more ideal. And since ws is already using JSDocs, I thought that this request would be feasible.

[lpinca]

Thank you, but I'm not interested. I don't want to deal with breaking changes in the types, issues opened for TypeScript related problems, adapt the JSDoc comments to please the TypeScript compiler, or replace the TypeScript "maintainers" once they are gone. Let all TypeScript stuff live outside the repository, please.

[ITenthusiasm]

If I may:

I don't want to deal with breaking changes in the types

I'm assuming that you're referring to when TypeScript updates. Is that correct? If so, then as Rich Harris said in the video, the TypeScript team is very cognizant of backwards compatibility. From what I can see, the ws library would basically be working with primitive types (and primitive types used in functions). I've never experienced breaking changes related to those in all my (~5) years of using TypeScript.

adapt the JSDoc comments to please the TypeScript compiler

This is something that I'd be willing to do (so you wouldn't have to), and it should be rather easy. For one thing, you technically wouldn't have to do this to get the compiler to work. (The minor tweaks would just be for making the types more specific and/or idiomatic.) For another thing, this is technically more advantageous for contributors because popular IDEs seem to understand TypeScript's types in JSDocs better.

replace the TypeScript "maintainers" once they are gone

You wouldn't have to. I think there are three thoughts worth considering:

1) The people currently maintaining @types/ws can simply maintain those types here. They can also take on the responsibility for triaging TS issues (you could assign those GitHub issues to them). I'm certain that they'd be more than happy to take care of this for you. (I'd be willing to help too.)

2) You wouldn't have to replace the type maintainers if you didn't want to. You've refused to take on type-related issues so far, and you can continue to do so in the future.

The reality is, if @types/ws goes dead, people are going to come to this repository to open TypeScript issues. It looks like that's already happened in the past. Because you're the maintainer of the ws package, TS users are always going to default to this repository. Automatically closing GitHub issues will not prevent this phenomenon from happening (whether the types are here or in @types/ws), but you have the freedom to ignore/decline those issues if you want -- just as you've been doing today.

3) You are already maintaining your JSDocs. There would not be a significant difference between maintaining those and maintaining the "different version". In fact, there's a high probability that the TS issues you encounter would decrease overall because your JSDocs would always be up to date.

Let all TypeScript stuff live outside the repository, please.

I will happily close this issue if you are genuinely unwilling to accept a tsconfig.json file in the repository, and if you are unwilling to receive help from the community. But respectfully, I think that you are perceiving this feature to bring vastly more effort/complexity than it actually requires. I would at least appreciate the opportunity to open a PR to show you how minimal the effort could be.

---

If you could just add a single tsconfig.json file to your repository and keep using your existing JSDocs, would you do that? You can already do that today. The additional (and minor) type changes would just be to improve the experience (for both you and your end users -- assuming you use a popular IDE).

[lpinca]

I will happily close this issue if you are genuinely unwilling to accept a tsconfig.json file in the repository, and if you are unwilling to receive help from the community. But respectfully, I think that you are perceiving this feature to bring vastly more effort/complexity than it actually requires.

Yes, I don't want it to be in the repository. The community is very active and useful one day and another day it no longer exists and you have to deal with problems you did not want to deal with in the first place. Thank you anyway.

I would at least appreciate the opportunity to open a PR to show you how minimal the effort could be.

I understand how small it would be but I'm still not ok with it. Once added, it would be hard to remove in the future.

[ITenthusiasm]

(Thank you so much for your time. Please allow me one or two more comments before closing.)

The community is very active and useful one day and another day it no longer exists and you have to deal with problems you did not want to deal with in the first place.

I would be willing to commit to maintaining the JSDoc types for at least 1 year. If at any point within that year (including during the initial PR) you conclude that the new JSDocs would make your workflow worse, and you conclude that they would only be a burden with no benefit whatsoever, then I will gladly take the responsibility for reverting those changes as well. (This is how much faith I have that the changes are easy to maintain.) Of course, if the PR that I create never gets merged, then nothing will have to be reverted anyway.

If I open a PR, then you would have the ability to test what your developer experience would be like in your own IDE with my fork of the repository. And you would get a clearer picture of the pros/cons. (If you aren't a fan of TS, then I think it's easier to assume from afar that things would be worse than they really are.)

Of course, if -- as I start writing out the code for the PR -- I somehow discover that the changes would yield more problems than I anticipated, then I could simply inform you of my discovery and not open the PR. In that case, things would remain exactly as they are today.

A test PR is the safest route in my opinion: You would get to test everything, and nothing would have to be reverted if the PR isn't merged.

I understand how small it would be but I'm still not ok with it. Once added it would be hard to remove in the future.

you have to deal with problems you did not want to deal with in the first place.

All that you would have to do to remove the feature is delete the tsconfig.json file from the repository, which is a very easy change. The minor updates to the JSDocs will always produce a better IDE experience, so I'm assuming that you would not want to revert those (nor would you need to).

I know you're saying that you understand the change would be small. But the fact that you're discussing future problems with maintenance leads me to think that you may still see the effort as larger than it really is. (I'm not just talking about the effort for adding the feature, I'm talking about the effort for maintaining/supporting/removing it afterwards.)

[lpinca]

If I open a PR, then you would have the ability to test what your developer experience would be like in your own IDE with my fork of the repository. And you would get a clearer picture of the pros/cons. (If you aren't a fan of TS, then I think it's easier to assume from afar that things would be worse than they really are.)

I would not notice because I do not take advantage of TS features in my workflow.

All that you would have to do to remove the feature is delete the tsconfig.json file from the repository, which is a very easy change.

The removal of the .d.ts file from the published package is a breaking change. Any change applied to it is potentially a breaking change.

Once the .d.ts file is added to the package, the amount of people that are not happy with it and want better types will sky rocket (this is based on experience from other packages).

If you want to improve JSDoc, go for it. If the goal is to add a .d.ts file to the published package, no thanks.

[ITenthusiasm]

The removal of the .d.ts file from the published package is a breaking change. Any change applied to it is potentially a breaking change.

I think generally, this is up to library authors. @types/* already can't follow semver because they try to make sure that the major version in @types/package matches the major version in package at all times. (This produces the least amount of confusion.) So when types change in your repo, you could choose not to do a major version bump.

Once the .d.ts file is added to the package, the amount of people that are not happy with it and want better types will sky rocket (this is based on experience from other packages).

I can't speak to this from experience, but this scenario sounds plausible. I could understand why that experience would not be desirable, though I personally think the better UX is worth it. (Yes, I know. It's much easier to say that when I'm not a maintainer.)

If you want to improve JSDoc, go for it. If the goal is to add a .d.ts file to the published package, no thanks.

Your concern about adding .d.ts files to the published package is reasonable. What about this: I could open a PR to improve the JSDocs. It would include a tsconfig.json file so that you could lint your codebase for type errors (better DX for you), but it would not generate .d.ts files (declaration: false).

If your repository is written in JSDocs, then in theory I should be able to follow a process like this:

1. Pull your code.
2. Change declaration: false to declaration: true.
3. Generate .d.ts files from your JSDoc code.
4. Publish the .d.ts files to @types/ws.

No one would have to know that the types "officially" came from here. That would be an "implementation detail" of how the maintainers of @types/ws decide to generate/publish the package's types.

• The burden still falls to the @types/ws maintainers to keep the package's types correct.
• You will still be able to decline type-related GitHub issues and redirect users to DefinitelyTyped.
• The maintainers of @types/ws would have an easier time because they could use this repository as a starting point while understanding that there is no guarantee of support/help for maintaining the @types/ws package or for generating types from ws's JSDocs. (That's basically how things are today since the repo is already written in JSDocs.)

Basically, you would get better types, and the @types/ws maintainers would ideally have an easier time. Again, you'd be able to reject my PR entirely (or otherwise alter it to suit what you prefer). Thoughts?

[lpinca]

Can't you keep the tsconfig.json file in your fork? I'm fine with the improved JSDoc part.

[ITenthusiasm]

Yes, I think that should be doable.

[ITenthusiasm]

In that case I'll rename this issue and start working on the JSDoc improvements as I have time.

[lpinca]

Ok.

[ITenthusiasm]

I'll probably have some questions as I work through this. I want to make sure that I understand what's going on in the code so that the type improvements that I try to make are valid. I'll probably have questions here and there. If this isn't the appropriate place to ask those, please let me know and direct me to the proper communication channel.

Regarding event-target.js:

• I see a lot of Symbols being used. It seems like these are used to simulate "private properties". Is that correct?
  • If my conclusion is correct, is there a reason for not using private class fields?
• The Event class says that the target property is any. Is that true? I believe that @types/ws says that target is a WebSocket instead. Is it possible for event.target to be anything else besides a WebSocket?

I also understand that these questions are type related and are not necessarily your highest priority. So don't feel pressured to answer immediately. (Though I would appreciate not being left hanging for like 3 months. 😅)

---

Edit: searching for the uses of EventTarget throughout your codebase, I see that only the addEventListener and removeEventListener properties are used from the mixin. (Indeed, those are the only two properties defined on that mixin.) And those properties are only added to WebSocket.prototype. This seems to confirm the suspicion that Event.target will only ever be of type WebSocket, but your explicit confirmation would still be helpful.

[lpinca]

If my conclusion is correct, is there a reason for not using private class fields?

Yes, it is correct. The reason is that Node.js 10 is still supported.

The Event class says that the target property is any. Is that true? I believe that @types/ws says that target is a WebSocket instead. Is it possible for event.target to be anything else besides a WebSocket.

It was made to be generic. A mixin can be used anywhere. It can only be WebSocket due to how it is currently used, but it could be any object technically.

[ITenthusiasm]

Yes, it is correct. The reason is that Node.js 10 is still supported.

✅

It was made to be generic. A mixin can be used anywhere. It can only be WebSocket due to how it is currently used, but it could be any object technically.

Is the desire to keep it generic in this way? From the JSDoc, it doesn't seem like EventTarget is intended for external use. So you wouldn't be bound by any requirements to keep event.target generic unless you wanted things to stay that way.

[lpinca]

Is the desire to keep it generic in this way?

Yes, I find it more correct. That stuff is not bound to WebSocket.

[ITenthusiasm]

Okay. Thank you. 🙏🏾

[ITenthusiasm]

The documentation says that an ErrorEvent's error property will always be some kind of Error object. But the code (and the JSDocs) seem to suggest that this value can be null sometimes.

Do you remember if ErrorEvent.error was supposed to be nullable or not?

---

Edit: There also seems to be a similar discrepancy between the documentation's MessageEvent.data property and the code's.

[lpinca]

It is nullable because an ErrorEvent can be created without passing the second argument or with an empty object as second argument. It never happens due to how it is used so the documentation is correct but yes, it can be null. The same applies to messageEvent.data.

[ITenthusiasm]

Is createWebSocketStream expected to operate only on ws's WebSocket class (or anything that conforms to that class's interface)?

MDN doesn't seem to suggest that a browser's WebSocket has methods like on.

[lpinca]

Is createWebSocketStream expected to operate only on ws's WebSocket class (or anything that conforms to that class's interface)?

Yes.

[ITenthusiasm]

Sender.frame claims to expect Buffer | string for its data parameter. But it passes this data value to mask (aliased to applyMask in the file), which claims to expect only a Buffer for its source parameter. Should mask expect Buffer | string instead?

Similar question when it comes to the discrepancy between Sender.frame's return type (Array<Buffer | string>) and the class's sendFrame method. (e.g., see https://github.com/websockets/ws/blob/master/lib/sender.js#L460)

---

Separate but related note, how important is the Sender class? The documentation doesn't directly provide information on its interface, but it is still exposed by the library. (Same question for the Receiver class.)

[lpinca]

Sender.frame claims to expect Buffer | string for its data parameter. But it passes this data value to mask (aliased to applyMask in the file), which claims to expect only a Buffer for its source parameter. Should mask expect Buffer | string instead?

The type of the data parameter is correct. When data is passed to applyMask it is guaranteed to be a Buffer.

Similar question when it comes to the discrepancy between Sender.frame's return type (Array<Buffer | string>) and the class's sendFrame method. (e.g., see https://github.com/websockets/ws/blob/master/lib/sender.js#L460)

The type of the list argument of sender.sendFrame() should be Array<Buffer | string>.

Separate but related note, how important is the Sender class? The documentation doesn't directly provide information on its interface, but it is still exposed by the library. (Same question for the Receiver class.)

They are not documented for various reasons. They are exposed for historic reasons and because they might be useful. For example, see #617 (comment).

[ITenthusiasm]

When data is passed to applyMask it is guaranteed to be a Buffer.

Forgive my ignorance, but on Line 119, the data argument is not converted to a Buffer. This suggested to me that data was still a string. Is this a special case where the string can behave like a Buffer?

Thanks again for all of your insights.

[lpinca]

See

ws/lib/sender.js

Line 153 in bfe1b2a

if (!options.mask) return [target, data];

and

ws/lib/sender.js

Line 161 in bfe1b2a

if (skipMasking) return [target, data];

. In that case the data is not masked (applyMask() is not called).

[ITenthusiasm]

Clever. Thanks.

[ITenthusiasm]

Based on the discussion in #2242, I think I'm going to close this issue. When I originally proposed my idea, I was thinking that there would at least be some TypeScript syntax in the JSDocs being used (preferably non-egregious TS syntax 😅). This syntax provides a better IDE experience without placing any constraints on maintainers, so I assumed that this would be desirable. However, after opening the PR, I now see that no TypeScript syntax or Google Closure Compiler type expression syntax is desired in this project. Since some degree of syntax from either of those tools would be necessary to implement the plan that I described earlier, this GitHub issue is sadly no longer actionable.

This is unfortunate since I consumed some of your time with my questions and with the PR that I opened. But nonetheless, I very much appreciate your willingness to answer those questions and review the PR. I'm sorry for the waste, and I'm very thankful for your patience.

Best!

[lpinca]

No problem. Thank you.