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

Simpler stream construction #108

Closed

Conversation

sonewman
Copy link
Contributor

This is a PR to support suggested functionality discussed in issue #102 to be discussed at WG Meeting: #106

  • Create prototype with ability to set required methods through stream construction
  • Discuss / review implementation with WG (@iojs/streams) members.
  • Finalise on best implementation
  • Add necessary unit tests to prove functionality

(Subsequent changes will likely be rebased into one commit)

Please review - Do Not Merge

@chrisdickinson
Copy link
Contributor

One thing that will be a prerequisite before this gets merged is to put in the work to reverse the flow of readable-stream into io.js.

@@ -121,6 +121,9 @@ function Readable(options) {
// legacy
this.readable = true;

if (options && util.isFunction(options.read))
this._read = options.read;

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I'm -0 on adding this if it's a shorthand for _read = fn – accepting functions into the constructor is a natural jumping off point for the other changes we'd like to see with streams, and if we don't take that opportunity when we add this functionality we'll have lost the chance to do so in the future (this roughly echoes my position from the other issue.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh so you were suggest jump straight to 'start' & 'pull'?

@sonewman
Copy link
Contributor Author

That's fine, it was more like a rough here's what I was suggesting, let's discuss. I don't think it is necessarily the final solution.

@sonewman
Copy link
Contributor Author

It's definitely best if we do this properly so we are all in agreement, I am just keen to prompt discussion.

@chrisdickinson
Copy link
Contributor

Per the WG meeting: everyone was pretty much +1 on this going in if the keys for _transform and _write don't conflict with WHATWG-style keys.

@sonewman
Copy link
Contributor Author

@chrisdickinson cool, well I'll wait for @domenic to come back with some feedback before writing some simple tests.

As you know I have raised some questions in (#102) maybe they were your initial concerns before?

@domenic
Copy link

domenic commented Feb 2, 2015

OK, so basically the semantics are compatible in all cases. (Except readable streams, but we dodge that by using read here which doesn't conflict with WHATWG streams start and pull.) This harmoniousness includes writev and transform/flush, which aren't finalized for WHATWG streams yet but I am pretty sure what they'll look like. All the important things are there: the same hooks are async in the same way, and roughly the same data is passed in.

In general the parameters don't match up, even ignoring the promise/callback issues. So people won't be able to transparently swap out new stream.Readable({...}) with new ReadableStream({ ... }). But we knew that would happen.

So, +1 to proceed.

@chrisdickinson
Copy link
Contributor

@domenic Awesome, thanks for looking into this.

@sonewman So, in the interest of expedience, you might re-open this PR over on iojs/io.js, and cc me there. We can merge it in there, then use the existing slurping tools to bring it into this repo. Great work!

@sonewman
Copy link
Contributor Author

sonewman commented Feb 2, 2015

@domenic @chrisdickinson awesome things for the update, I will knock up some tests and raised as a PR in iojs/io.js also.

@sonewman sonewman changed the title Simplier stream construction Simpler stream construction Feb 3, 2015
@sonewman
Copy link
Contributor Author

This is in core, so I am going to close

@sonewman sonewman closed this Feb 13, 2015
@sonewman sonewman deleted the simplier-stream-construction branch February 13, 2015 19:45
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.

3 participants