Skip to content

Conversation

@MoonBall
Copy link
Member

@MoonBallMoonBall commented Jan 21, 2018

I found that the readable stream that isn't in object mode supports undefinedlong 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-botnodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jan 21, 2018
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
MemberAuthor

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
MemberAuthor

Choose a reason for hiding this comment

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

@jasnell fixed

@MoonBallMoonBallforce-pushed the doc-add-readable.push-supports-undefined branch from 63ec723 to 5ec2d5aCompareJanuary 22, 2018 16:09
@BridgeAR
Copy link
Member

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
MemberAuthor

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

@BridgeAR
Copy link
Member

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

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.

Copy link
MemberAuthor

@MoonBallMoonBallFeb 23, 2018

Choose a reason for hiding this comment

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

Copy link
MemberAuthor

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

@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

@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

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.

@MoonBallMoonBallforce-pushed the doc-add-readable.push-supports-undefined branch from 5ec2d5a to c17c7ebCompareFebruary 23, 2018 06:48
Copy link
Member

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

@mcollina
Copy link
Member

Landing.

@mcollina
Copy link
Member

Landed in 8b518ed

mcollina pushed a commit that referenced this pull request Feb 26, 2018
`readable.push()` supports `undefined` in non-object mode, but it was not previously documented. PR-URL: #18283 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Feb 26, 2018
`readable.push()` supports `undefined` in non-object mode, but it was not previously documented. PR-URL: nodejs#18283 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
`readable.push()` supports `undefined` in non-object mode, but it was not previously documented. PR-URL: #18283 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@addaleaxaddaleax mentioned this pull request Feb 27, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
`readable.push()` supports `undefined` in non-object mode, but it was not previously documented. PR-URL: nodejs#18283 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
jasnell pushed a commit to jasnell/node that referenced this pull request Aug 17, 2018
`readable.push()` supports `undefined` in non-object mode, but it was not previously documented. PR-URL: nodejs#18283 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 6, 2018
`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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Sep 6, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@MoonBall@BridgeAR@mcollina@jasnell@nodejs-github-bot