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

Socket onread described on wrong function #53792

Closed
lights0123 opened this issue Jul 9, 2024 · 2 comments · Fixed by #54194
Closed

Socket onread described on wrong function #53792

lights0123 opened this issue Jul 9, 2024 · 2 comments · Fixed by #54194
Labels
doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem.

Comments

@lights0123
Copy link

Affected URL(s)

https://nodejs.org/api/net.html#new-netsocketoptions

Description of the problem

socket.connect() is described as accepting the following option:

  • onread {Object} If specified, incoming data is stored in a single buffer
    and passed to the supplied callback when data arrives on the socket.
    This will cause the streaming functionality to not provide any data.
    The socket will emit events like 'error', 'end', and 'close'
    as usual. Methods like pause() and resume() will also behave as
    expected.
    • buffer {Buffer|Uint8Array|Function} Either a reusable chunk of memory to
      use for storing incoming data or a function that returns such.
    • callback {Function} This function is called for every chunk of incoming
      data. Two arguments are passed to it: the number of bytes written to
      buffer and a reference to buffer. Return false from this function to
      implicitly pause() the socket. This function will be executed in the
      global context.

However, this is not correct, and is actually accepted only in the Socket() constructor:

node/lib/net.js

Line 454 in b4e8f1b

if (onread !== null && typeof onread === 'object' &&

The example given using net.connect() is still correct because the options "Will be passed to both the new net.Socket([options]) call and the socket.connect(options[, connectListener]) method."

@lights0123 lights0123 added the doc Issues and PRs related to the documentations. label Jul 9, 2024
@RedYetiDev
Copy link
Member

Thanks for the report! If you have an improvement, feel free to submit a PR!

@RedYetiDev RedYetiDev added the net Issues and PRs related to the net subsystem. label Jul 9, 2024
@sophob
Copy link
Contributor

sophob commented Jul 18, 2024

The example given using net.connect() is still correct because the options "Will be passed to both the new net.Socket([options]) call and the socket.connect(options[, connectListener]) method."

@lights0123 yes, it's still correct because net.connect(options[, connectListener]) create an instance of Socket(options) when it called

node/lib/net.js

Line 227 in b4e8f1b

const socket = new Socket(options);

I think a solution can be to move this function description to Socket class and mention on the net.connect(options[, connectListener]) part it's can be possible to use same onread object from Socket class constructor. Right ? @RedYetiDev what do you think about it ?

sendoru added a commit to sendoru/node that referenced this issue Aug 3, 2024
sendoru added a commit to sendoru/node that referenced this issue Aug 3, 2024
targos pushed a commit that referenced this issue Aug 14, 2024
Fixes: #53792
PR-URL: #54194
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
targos pushed a commit that referenced this issue Sep 21, 2024
Fixes: #53792
PR-URL: #54194
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
targos pushed a commit that referenced this issue Oct 2, 2024
Fixes: #53792
PR-URL: #54194
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants