-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
doc: readable.push()
supports undefined
in non-object mode
#18283
doc: readable.push()
supports undefined
in non-object mode
#18283
Conversation
doc/api/stream.md
Outdated
@@ -1797,6 +1797,10 @@ class SourceWrapper extends Readable { | |||
*Note*: The `readable.push()` method is intended be called only by Readable | |||
Implementers, and only from within the `readable._read()` method. | |||
|
|||
*Note*: For streams not operating in object mode, if the `chunk` parameter | |||
of `readable.push()` is `undefined`, it will be treated as empty string or | |||
buffer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps just remove the *Note*:
part, it's really not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because I don't know where it should be placed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasnell fixed
63ec723
to
5ec2d5a
Compare
Hm, I personally wonder if we really should support undefined. I liked #18244. If someone pushes |
I also think that no support for |
@mcollina @mafintosh what do you two think about removing the support? Is there a good reason to support |
doc/api/stream.md
Outdated
@@ -1797,6 +1797,10 @@ class SourceWrapper extends Readable { | |||
*Note*: The `readable.push()` method is intended be called only by Readable | |||
Implementers, and only from within the `readable._read()` method. | |||
|
|||
For streams not operating in object mode, if the `chunk` parameter of | |||
`readable.push()` is `undefined`, it will be treated as empty string or | |||
buffer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would mean for the chunk
to be interpreted as an empty string of buffer?
Can you add another sentence explaining what is happening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MoonBall ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcollina See also: readable.push('')
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcollina I added a sentence for readable.push('')
.
@BridgeAR there isn't. However, I do not see why removing that support would improve our API, so why breaking things? |
@mcollina the main point for me would be that pushing undefined might be a indicator for having a bug in the code and by prohibiting that it could unveil those bugs. |
let's land this and then we can fire the semver-major thing. |
@mcollina I would say we either document it or we want to deprecate it. But documenting something that we do not really want does not make a lot of sense to me. |
Removing or deprecating things are semver-major. A doc change can be easily backported. |
@mcollina I am aware that it is easy to backport a doc change but for me the question is: do we really want to document this? |
This has been part of streams for a very long time. We can consider it public knowledge and state the fact that this is supported behavior. |
5ec2d5a
to
c17c7eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Landing. |
Landed in 8b518ed |
`readable.push()` supports `undefined` in non-object mode, but it was not previously documented. PR-URL: #18283 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
`readable.push()` supports `undefined` in non-object mode, but it was not previously documented. PR-URL: nodejs#18283 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
`readable.push()` supports `undefined` in non-object mode, but it was not previously documented. PR-URL: #18283 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
`readable.push()` supports `undefined` in non-object mode, but it was not previously documented. PR-URL: nodejs#18283 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
`readable.push()` supports `undefined` in non-object mode, but it was not previously documented. PR-URL: nodejs#18283 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
I found that the readable stream that isn't in object mode supports
undefined
long ago, and it's original idea is to express EOF. Now it is just treated as a empty string or buffer. But the doc and code aren't clear for it although it is not important.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
doc, stream