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: clarify unshift EOF #29950

Closed
wants to merge 2 commits into from
Closed

doc: clarify unshift EOF #29950

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Oct 13, 2019

We should not allow unshifting EOF.

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

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. stream Issues and PRs related to the stream subsystem. labels Oct 13, 2019
@Fishrock123
Copy link
Contributor

@ronag Could you please describe what the present behavior is? (i.e. before this change.)

@ronag
Copy link
Member Author

ronag commented Oct 15, 2019

@Fishrock123: Previously unshift(null) would be the same as push(null).

@Fishrock123
Copy link
Contributor

As in, end the stream? It seems reasonable then to also accept unshift to end the stream in an ‘expedited’ way?

@ronag
Copy link
Member Author

ronag commented Oct 15, 2019

As in, end the stream? It seems reasonable then to also accept unshift to end the stream in an ‘expedited’ way?

Well, it's not really 'expedited' since it is exactly the same as push(null).

Unshifting "eof" sounds very weird to me. Though, now that you question it there is probably no harm in it either... I don't mind closing this.

@jasnell
Copy link
Member

jasnell commented Oct 15, 2019

I think there would at least be value in documenting

@Fishrock123 Fishrock123 requested review from mcollina and removed request for mcollina October 16, 2019 01:27
@Fishrock123
Copy link
Contributor

I think we should document it, and then think about if you should be able to actually unshift EOF so that it comes before other data.

@Trott
Copy link
Member

Trott commented Oct 16, 2019

@nodejs/streams

@mcollina
Copy link
Member

Why we should not unshift EOF?

@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 16, 2019
@ronag
Copy link
Member Author

ronag commented Oct 16, 2019 •
edited
Loading

Why we should not unshift EOF?

I guess the confusion is, what does it mean to unshift EOF? Right now it behaves exactly like push EOF which is unexpected/confusing/undefined.

mcollina
mcollina requested changes Oct 16, 2019
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.

I do not see a compelling reason to change this.

@ronag
Copy link
Member Author

ronag commented Oct 16, 2019 •
edited
Loading

I changed this to only a doc update. @Fishrock123

@ronag ronag force-pushed the stream-fix-unshift-eof branch from 20a6c34 to 4f73edf Compare October 16, 2019 10:55
@mcollina mcollina removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 16, 2019
mcollina
mcollina approved these changes Oct 16, 2019
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

Trott
Trott reviewed Oct 16, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
@ronag ronag changed the title stream: no unshift eof doc: clarify unshift EOF Oct 16, 2019
@ronag ronag force-pushed the stream-fix-unshift-eof branch from 7d15e65 to e184520 Compare October 16, 2019 18:52
@Trott
Copy link
Member

Trott commented Oct 16, 2019

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3999/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 16, 2019
@Fishrock123 Fishrock123 added doc Issues and PRs related to the documentations. and removed errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Oct 16, 2019
@Trott
Copy link
Member

Trott commented Oct 16, 2019

Landed in 273d38b

Trott pushed a commit that referenced this pull request Oct 16, 2019
@ronag @Trott
doc: clarify readable.unshift null/EOF
273d38b 
PR-URL: #29950
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Trott Trott closed this Oct 16, 2019
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
@ronag @MylesBorins
doc: clarify readable.unshift null/EOF
e17892c 
PR-URL: #29950
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
@ronag @MylesBorins
doc: clarify readable.unshift null/EOF
62b4ca6 
PR-URL: #29950
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 23, 2019
Merged
targos pushed a commit that referenced this pull request Nov 8, 2019
@ronag @targos
doc: clarify readable.unshift null/EOF
fa3f5f8 
PR-URL: #29950
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
targos pushed a commit that referenced this pull request Nov 10, 2019
@ronag @targos
doc: clarify readable.unshift null/EOF
5616f22 
PR-URL: #29950
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@targos targos mentioned this pull request Nov 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@Trott Trott Trott left review comments

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
7 participants
@ronag @Fishrock123 @jasnell @Trott @mcollina @richardlau @nodejs-github-bot