Skip to content

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