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

Improve JSDoc Types (Part 1) #2242

Merged

Conversation

ITenthusiasm
Copy link
Contributor

@ITenthusiasm ITenthusiasm commented Jul 13, 2024

Description

This is an initial amount of work for improving the JSDocs in the codebase (as mentioned in #2241).


I figured it would make more sense to apply the JSDoc improvements incrementally. This has a few benefits:

  • PRs end up being smaller to review (compared to if everything was done at once)
  • Maintainers can more easily determine if [Feature]: Improve Project's JSDoc Types #2241 is worth pursuing (in case they want to cancel everything or only accept fragments of the work)
  • Contributors can make progress more asynchronously (in case there are gaps in free time)

Note: If the preference is only to merge everything at once, then this PR can be kept in a Draft State until it is "sufficiently complete".

Additional Notes

When using JSDocs, TypeScript is fine with using String or string, * or any, and the like interchangeably. Not as idiomatic, but I'm not focused on that at the moment.

lib/event-target.js Outdated Show resolved Hide resolved
lib/buffer-util.js Outdated Show resolved Hide resolved
@lpinca
Copy link
Member

lpinca commented Jul 13, 2024

Before you go any further, JSDoc comments should be kept as valid JSDoc, no TypeScript specific syntax.

@ITenthusiasm
Copy link
Contributor Author

Noted. I'll try to see what I can do.

lib/event-target.js Outdated Show resolved Hide resolved
@ITenthusiasm
Copy link
Contributor Author

ITenthusiasm commented Jul 13, 2024

For clarity, it seems like JSDoc supports Google Closure Compiler type expressions. Are we also interested in having those (e.g., generics via @template) here? Or no?

(I'm just trying to figure out what to remove/filter out, e.g., ConstructorParameters, Parameters, typeof.)

@lpinca
Copy link
Member

lpinca commented Jul 13, 2024

I don't think @template exists in JSDoc.

@ITenthusiasm ITenthusiasm force-pushed the it/jsdoc-improvements-without-tsconfig branch 4 times, most recently from d2db6eb to 797c742 Compare July 13, 2024 20:06
@ITenthusiasm
Copy link
Contributor Author

ITenthusiasm commented Jul 13, 2024

Okay. The PR has become somewhat anticlimactic, but I think that all of the TypeScript syntax should be gone now.

lib/stream.js Outdated Show resolved Hide resolved
@ITenthusiasm ITenthusiasm force-pushed the it/jsdoc-improvements-without-tsconfig branch from 797c742 to 3ebc5cf Compare July 13, 2024 20:19
@ITenthusiasm
Copy link
Contributor Author

The update to sendFrame is all that's left. Pretty simple 👍🏾

@ITenthusiasm
Copy link
Contributor Author

Made the suggested change for createWebSocketStream. Can remove if desired.

@lpinca lpinca merged commit 019f28f into websockets:master Jul 14, 2024
56 checks passed
@ITenthusiasm ITenthusiasm deleted the it/jsdoc-improvements-without-tsconfig branch July 14, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants