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

Declare correct ws options type #338

Merged
merged 3 commits into from
Jan 8, 2025
Merged

Declare correct ws options type #338

merged 3 commits into from
Jan 8, 2025

Conversation

dominykas
Copy link
Contributor

@dominykas dominykas commented Jan 6, 2025

The original string | string[] seems to have been a confusion because the ws package client takes 3 arguments, of which the middle one (protocols) is the string | string[], but it is optional. When it is left out - it becomes the options object instead: https://github.com/websockets/ws/blob/c798dd4ee20efb2d7591b5659839ad05cdb3eb70/lib/websocket.js#L80

The type information is covered in jsdoc: https://github.com/websockets/ws/blob/c798dd4ee20efb2d7591b5659839ad05cdb3eb70/lib/websocket.js#L627

A more complete typing information is exposed by @types/ws, though.

Closes #337.

The original string | string[] seems to have been a mistake because the ws package client takes 3 arguments, of which the middle one (protocols) is the string | string[], but it is optional. When it is left out - it becomes the options object instead: https://github.com/websockets/ws/blob/c798dd4ee20efb2d7591b5659839ad05cdb3eb70/lib/websocket.js#L80

The type information is covered in jsdoc: https://github.com/websockets/ws/blob/c798dd4ee20efb2d7591b5659839ad05cdb3eb70/lib/websocket.js#L627

A more complete typing information is exposed by @types/ws, though.
@dominykas dominykas changed the title Declare correct ws options type Declare correct ws options type Jan 6, 2025
@damusix
Copy link
Contributor

damusix commented Jan 6, 2025

@dominykas Hey Dom, thanks for the PR. Two things:

  1. Can you run type tests and resolve the type errors?
  2. Can you add a test that validates this change of typings? A config with that typing will do. Validate that it'll connect and send a message.

@dominykas
Copy link
Contributor Author

🤔 this is also failing on current master - probably unrelated to my change, but I'll take a look.

@dominykas
Copy link
Contributor Author

dominykas commented Jan 7, 2025

  1. Checked the details. Reverting 2132240 (basically, lab upgrade) fixes the build. The error reported comes from node_modules/@hapi/hapi/lib/types/server/events.d.ts:147:17 i.e. from Hapi itself - so the fix probably belongs there? In fact, type tests on the next branch of Hapi are failing there, although given that the types haven't changed, it's a types bug on master (even if those are not being tested in lab@25 on master).
  2. Ah, I was not aware these exist 👍 plucked some examples from existing tests: 66dd37a

Seems there's also an unrelated test failing in node 22...

@Marsup
Copy link
Contributor

Marsup commented Jan 7, 2025

Fixed the type issue in podium hapijs/podium#84. It looked like an innocent change, sorry for that.

@Marsup
Copy link
Contributor

Marsup commented Jan 7, 2025

It should be fine now, also for node 23.

@dominykas
Copy link
Contributor Author

Thanks!

Resolved conflicts - things are now green.

@Marsup Marsup added this to the 14.0.1 milestone Jan 8, 2025
@Marsup Marsup self-assigned this Jan 8, 2025
@Marsup Marsup added the types TypeScript type definitions label Jan 8, 2025
@Marsup Marsup merged commit ccf86f9 into hapijs:master Jan 8, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types TypeScript type definitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type information for client constructor incorrect
3 participants