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.from with null #32845

Closed
ronag opened this issue Apr 14, 2020 · 2 comments
Closed

stream: Readable.from with null #32845

ronag opened this issue Apr 14, 2020 · 2 comments

Comments

@ronag
Copy link
Member

ronag commented Apr 14, 2020

Reading through the Readable.from code is seems to me that the semantics of using Readable.from on a generator which yields null is not entirely clear.

Should we add a check for value == null and throw?

  async function next() {
    try {
      const { value, done } = await iterator.next();
      if (done) {
        readable.push(null);
      } else if (readable.push(await value)) { // Hm, push(null) and then next()?
        next();
      } else {
        reading = false;
      }
    } catch (err) {
      readable.destroy(err);
    }
  }

Might also want to be a bit more explicit in the docs about this?

@rexagod
Copy link
Member

rexagod commented Apr 15, 2020

This makes sense. @ronag can I work on this?

@ronag
Copy link
Member Author

ronag commented Apr 15, 2020

Go for it.

rexagod added a commit to rexagod/node that referenced this issue Apr 16, 2020
Throws `ERR_STREAM_NULL_VALUES` error if a null value is passed to
`Readable.from`. Also added docs for the same.

Fixes: nodejs#32845
rexagod added a commit to rexagod/node that referenced this issue Apr 28, 2020
Throws `ERR_STREAM_NULL_VALUES` error if a null value is passed to
`Readable.from`. Also added docs for the same.

Fixes: nodejs#32845

resolve recieved value and add test

Update test/parallel/test-stream-readable-next-no-null.js

Co-Authored-By: 扩散性百万甜面包 <[email protected]>

rebase fix

fixup

fixup: destroy -> throw
targos pushed a commit that referenced this issue May 4, 2020
Throws `ERR_STREAM_NULL_VALUES` error if a null value is passed to
`Readable.from`. Also added docs for the same.

Co-Authored-By: 扩散性百万甜面包 <[email protected]>
Fixes: #32845
PR-URL: #32873
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this issue May 7, 2020
Throws `ERR_STREAM_NULL_VALUES` error if a null value is passed to
`Readable.from`. Also added docs for the same.

Co-Authored-By: 扩散性百万甜面包 <[email protected]>
Fixes: #32845
PR-URL: #32873
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this issue May 13, 2020
Throws `ERR_STREAM_NULL_VALUES` error if a null value is passed to
`Readable.from`. Also added docs for the same.

Co-Authored-By: 扩散性百万甜面包 <[email protected]>
Fixes: #32845
PR-URL: #32873
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants