Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

bugfix: Type improvements on "port" option in Socket and SocketOption… #680

Conversation

kaangokdemir
Copy link
Contributor

@kaangokdemir kaangokdemir commented Oct 31, 2021

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

port: string;

New behaviour

port: string | number;

@darrachequesne
Copy link
Member

Thanks for this 👍

I'm wondering if it would make sense to remove the string and use only number, like here. WDYT?

@kaangokdemir
Copy link
Contributor Author

kaangokdemir commented Nov 3, 2021

@darrachequesne I'm not sure since the port of browser's location object returns a string. Check that:

https://developer.mozilla.org/en-US/docs/Web/API/Location/port

@darrachequesne darrachequesne merged commit 8f68f77 into socketio:master Nov 5, 2021
@darrachequesne
Copy link
Member

Well, that makes sense. Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants