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.

Sign up for GitHub

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

Jump to bottom

doc: readable.push() supports undefined in non-object mode #18283

Closed
wants to merge 1 commit into from
Closed

doc: readable.push() supports undefined in non-object mode #18283

wants to merge 1 commit into from

Conversation

MoonBall
Copy link
Member

@MoonBall MoonBall commented Jan 21, 2018 •
edited
Loading

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), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc, stream

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jan 21, 2018
jasnell
jasnell reviewed Jan 22, 2018
View reviewed changes
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.
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell fixed

@MoonBall MoonBall force-pushed the doc-add-readable.push-supports-undefined branch from 63ec723 to 5ec2d5a Compare January 22, 2018 16:09
@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

Hm, I personally wonder if we really should support undefined.

I liked #18244. If someone pushes undefined I guess in most cases it will actually be a mistake or is there a use case where it makes sense to push undefined that I just can not think about?

@MoonBall
Copy link
Member Author

MoonBall commented Feb 2, 2018

I also think that no support for undefined is better.
But I am afraid that it maybe have other influences.

@BridgeAR
Copy link
Member

BridgeAR commented Feb 6, 2018

@mcollina @mafintosh what do you two think about removing the support? Is there a good reason to support undefined?

mcollina
mcollina reviewed Feb 7, 2018
View reviewed changes
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.
Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MoonBall ping

Copy link
Member Author

@MoonBall MoonBall Feb 23, 2018 •
edited
Loading

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('').

Copy link
Member Author

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('').

@mcollina
Copy link
Member

mcollina commented Feb 7, 2018

@BridgeAR there isn't. However, I do not see why removing that support would improve our API, so why breaking things?

@BridgeAR
Copy link
Member

BridgeAR commented Feb 7, 2018

@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.

@mcollina
Copy link
Member

mcollina commented Feb 7, 2018

let's land this and then we can fire the semver-major thing.

@BridgeAR
Copy link
Member

@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.

@mcollina
Copy link
Member

@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.

@BridgeAR
Copy link
Member

@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?
I would say no in case we want to deprecate it.

@mcollina
Copy link
Member

@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?
I would say no in case we want to deprecate it.

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.

@MoonBall MoonBall force-pushed the doc-add-readable.push-supports-undefined branch from 5ec2d5a to c17c7eb Compare February 23, 2018 06:48
mcollina
mcollina approved these changes Feb 23, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/13347/

@mcollina
Copy link
Member

Landing.

@mcollina
Copy link
Member

Landed in 8b518ed

@mcollina mcollina closed this Feb 26, 2018
mcollina pushed a commit that referenced this pull request Feb 26, 2018
@mcollina
doc: readable.push(undefined) in non-object mode
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>
addaleax pushed a commit to addaleax/node that referenced this pull request Feb 26, 2018
@addaleax
doc: readable.push(undefined) in non-object mode
eb4ab48 
`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>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
@addaleax
doc: readable.push(undefined) in non-object mode
6f177e7 
`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>
@addaleax addaleax mentioned this pull request Feb 27, 2018
Closed
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@MayaLekova
doc: readable.push(undefined) in non-object mode
3f4b50b 
`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>
jasnell pushed a commit to jasnell/node that referenced this pull request Aug 17, 2018
@jasnell
doc: readable.push(undefined) in non-object mode
08ccba7 
`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>
MylesBorins pushed a commit that referenced this pull request Sep 6, 2018
@MylesBorins
doc: readable.push(undefined) in non-object mode
dc6dadd 
`readable.push()` supports `undefined` in non-object mode, but it was
not previously documented.

Backport-PR-URL: #22380
PR-URL: #18283
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 6, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MoonBall @BridgeAR @mcollina @jasnell @nodejs-github-bot