Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

stream: readable stream should not push undefined in non-objectMode#18244

Closed
MoonBall wants to merge 2 commits intonodejs:masternodejs/node:masterfrom
MoonBall:readable-stream-should-not-push-undefined-in-non-objectModeMoonBall/node:readable-stream-should-not-push-undefined-in-non-objectModeCopy head branch name to clipboard
Closed

stream: readable stream should not push undefined in non-objectMode#18244
MoonBall wants to merge 2 commits intonodejs:masternodejs/node:masterfrom
MoonBall:readable-stream-should-not-push-undefined-in-non-objectModeMoonBall/node:readable-stream-should-not-push-undefined-in-non-objectModeCopy head branch name to clipboard

Conversation

@MoonBall
Copy link
Member

@MoonBall MoonBall commented Jan 19, 2018 •
edited
Loading

From the doc:
image
But current Node.js allows we to push undefined when stream is in non-object mode.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

stream

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jan 19, 2018
@MoonBall MoonBall mentioned this pull request Jan 19, 2018
Closed
4 tasks
@mcollina
Copy link
Member

mcollina commented Jan 19, 2018

Why do we need it? I'm not understanding the reasoning behind this.

@MoonBall
Copy link
Member Author

MoonBall commented Jan 19, 2018 •
edited
Loading

Current Node.js allows we to push undefined when stream is in non-object mode.
I think pushing undefined should not be allowed when stream is in non-object mode, so I made this change.

@addaleax
Copy link
Member

addaleax commented Jan 19, 2018

@MoonBall It would be helpful if you could explain why you think that that should not be allowed?

@MoonBall
Copy link
Member Author

MoonBall commented Jan 19, 2018 •
edited
Loading

There is no obvious reason. just feeling.
Why do we allow it?
Is the reason that we treat undefined to empty string or buffer?
If so, I can also accept it.

@MoonBall
Copy link
Member Author

MoonBall commented Jan 19, 2018 •
edited
Loading

@addaleax If we allow it, I can add a test case and a comment to explain the reason.
It surely gives me some confusion.

@MoonBall
Copy link
Member Author

MoonBall commented Jan 19, 2018

@addaleax I found the doc.
image

@MoonBall MoonBall force-pushed the readable-stream-should-not-push-undefined-in-non-objectMode branch from ba704a3 to a186f78 Compare January 21, 2018 16:11
@MoonBall
Copy link
Member Author

MoonBall commented Jan 21, 2018 •
edited
Loading

@mcollina @addaleax I found that readable.push() supported undefined in normal mode long ago. The PR can be closed, and I will modify the doc to state it.

@mcollina mcollina closed this Jan 22, 2018
@mcollina
Copy link
Member

mcollina commented Jan 22, 2018

Thanks for the investigation!

@BridgeAR BridgeAR mentioned this pull request Feb 1, 2018
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

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.

4 participants

@MoonBall @mcollina @addaleax @nodejs-github-bot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.