★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/29950

Nouvelle recherche | Portail wanayoo

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

doc: clarify unshift EOF #29950

Closed

ronag wants to merge 2 commits into nodejs:master from nxtedition:stream-fix-unshift-eof

Contributor

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 added errors stream labels

Member

Fishrock123 commented Oct 15, 2019

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

Contributor Author

ronag commented Oct 15, 2019

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

Member

Fishrock123 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?

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

Member

jasnell commented Oct 15, 2019

I think there would at least be value in documenting

Fishrock123 requested review from mcollina and removed request for mcollina

Member

Fishrock123 commented Oct 16, 2019

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.

Member

Trott commented Oct 16, 2019

@nodejs/streams

Member

mcollina commented Oct 16, 2019

Why we should not unshift EOF?

mcollina added the semver-major label

Contributor Author

ronag commented Oct 16, 2019 •

edited

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 requested changes

Member

mcollina left a comment

I do not see a compelling reason to change this.


        stream: clearify readable.unshift null

4f73edf

Contributor Author

ronag commented Oct 16, 2019 •

edited

I changed this to only a doc update. @Fishrock123

ronag force-pushed the nxtedition:stream-fix-unshift-eof branch from 20a6c34 to 4f73edf

mcollina removed the semver-major label

mcollina approved these changes

Member

mcollina left a comment

LGTM

Trott reviewed

doc/api/stream.md Outdated Show resolved Hide resolved


        doc: clarify unshift EOF

e184520

ronag changed the title ~~stream: no unshift eof~~ doc: clarify unshift EOF

ronag force-pushed the nxtedition:stream-fix-unshift-eof branch from 7d15e65 to e184520

Member

Trott commented Oct 16, 2019

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

Trott added the author ready label

Fishrock123 approved these changes

Fishrock123 added doc and removed errors labels

Member

Trott commented Oct 16, 2019

Landed in 273d38b

Trott added a commit that referenced this pull request


        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 closed this

MylesBorins added a commit that referenced this pull request


        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 added a commit that referenced this pull request


        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 mentioned this pull request

v13.0.1 proposal #30081

Merged

targos added a commit that referenced this pull request


        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 added a commit that referenced this pull request


        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 mentioned this pull request

v12.13.1 release proposal #30352

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

You can’t perform that action at this time.