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

Check 'end' and 'error' events in fromStream() #2

Closed
wants to merge 2 commits into from

Conversation

novemberborn
Copy link

Remove access to private _readableState, which may be missing from third-party stream implementations.

Assumes non-ended streams are passed, in paused mode, in which case end is emitted after read() is called.

Fixes #1.

I realize the commit messages are not in the expected format, but I reckon you can fix that in a squash merge 😉

Copy link
Owner

@reconbot reconbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sweet, I’ll check it out as soon as I can but it reads really well.

@reconbot
Copy link
Owner

I don't like this for one reason

Assumes non-ended streams

The reason is no other iterators have this restriction, and I'd rather special case the various kinds of streams than force a different model.

For example

function* numbers () {
  yield 1
  yield 2
  yield 3
}

const nums = numbers()

for (const num of nums) {
  console.log('should print', num)
}

for (const num of nums) {
  console.log(`shouldn't print`, num)
}

However you left the test iterates empty streams and it still passes? It looks like I have to setup CI since the repo moved.

@reconbot
Copy link
Owner

rebase/merge master and you'll have CI

Remove access to private _readableState, which may be missing from third-party stream implementations.

Assumes non-ended streams are passed, in paused mode, in which case 'end' is emitted after read() is called.

Fixes reconbot#1.
@novemberborn
Copy link
Author

I don't like this for one reason

Assumes non-ended streams

The reason is no other iterators have this restriction, and I'd rather special case the various kinds of streams than force a different model.

Before Node.js 12.9.0 there was no sanctioned API for determining whether a stream had ended. Happy to add that as a check, though this assumption would still apply to non-conformant streams. We could retain the previous check as well, though with some guards to avoid crashes.

I don't see how this relates to other iterators.

you left the test iterates empty streams and it still passes?

That just happens to be how pass-through streams work. The end event isn't actually emitted until you call read(), which we do after hooking up the listener.

@novemberborn
Copy link
Author

I've rebased but I'm not sure if GitHub is picking up the CI run. Maybe because this PR predates it. It may work if you push these commits to the main repo.

@reconbot
Copy link
Owner

reconbot commented Jul 8, 2020

Hey sorry this drifted a bit since you opened it. I wasn’t able to reason about it at the time and I’m not sure I like the assumption it requires. So I’m going to close it for now.

@reconbot reconbot closed this Jul 8, 2020
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 this pull request may close these issues.

Possible improvements to fromStream()
2 participants