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

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

Conversation

MoonBall
Copy link
Member

@MoonBall MoonBall commented Jan 19, 2018

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
@mcollina
Copy link
Member

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

@MoonBall
Copy link
Member Author

MoonBall commented Jan 19, 2018

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

@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

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

@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

@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

@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

Thanks for the investigation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants