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

fix: add rinfo to UDPHelper message event #103

Merged
merged 6 commits into from
Nov 18, 2024
Merged

Conversation

Luuccc
Copy link
Contributor

@Luuccc Luuccc commented Nov 10, 2024

  • Added rinfo argument to message event as it was omitted.
  • Added emit to 'message' event for consistency.
  • Including original 'data' event for projects that expect it, as
    well as rinfo.
  • Renamed data arg to msg, inline with nodejs api.

 - Moved `setBroadcast(true)` into 'listening' event to better ensure
that bind() has been completed successfully, resolving EBADF error
 - Added rinfo argument to message event as it was omitted.
 - Added emit to 'message' event for consistency.
 - Including original 'data' event for projects that expect it, as
well as rinfo.
 - Renamed data arg to msg, inline with nodejs api.
@Luuccc
Copy link
Contributor Author

Luuccc commented Nov 10, 2024

Interesting, I sort of understand why it might be best to leave this.emit('data', msg) as it was originally. Even though as I understand it, the added parameter is optional.

But I'm unsure why the Argument of type '"message"' is not assignable to parameter of type 'keyof UDPHelperEvents'.

@Julusian
Copy link
Member

I don't think we should emit both 'data' and 'message' events. I expect that will lead to confusion from module devs questioning what the difference is and which they should use.
While our naming doesn't match the nodejs dgram, it is consistent with the TCPHelper

That typings issue is because you need to update the typescript interface here:

data: [msg: Buffer]

That line will need updating to include the new parameter with the correct type

 - Removed possibly confusing suggestion to emit to 'message'
 - Included rinfo parameter to event and emit
 - Updated data packet to include rinfo Object
@Luuccc
Copy link
Contributor Author

Luuccc commented Nov 17, 2024

I agree that might cause some confusion.
I've removed the emit to an external 'message' event, and I've fixed the issue with the interface

src/helpers/udp.ts Outdated Show resolved Hide resolved
@Julusian Julusian changed the title fix: Extended event to include rinfo fix: add rinfo to UDPHelper message event Nov 18, 2024
@Julusian Julusian merged commit d7162c3 into bitfocus:main Nov 18, 2024
3 checks passed
@Julusian
Copy link
Member

This is in v1.11.2

@Luuccc
Copy link
Contributor Author

Luuccc commented Nov 18, 2024

Thanks Julian, and also for the code review. I never would've guessed it's type being dgram.RemoteInfo, I can't find a mention of it anywhere, beyond some mentions within net.js and this definition issue. Thanks again

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