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: simpler stream constructon #697

Closed
wants to merge 5 commits into from

Conversation

sonewman
Copy link
Contributor

@sonewman sonewman commented Feb 3, 2015

This allows stream implementers the advantage of being able to pass the necessary stream specific methods to the streams constructor as part of its options upon construction.

Referenced to discussion in issue nodejs/readable-stream#102 of iojs/readable-stream

Please review.

cc: @chrisdickinson @iojs/streams

Via revealing constructor pattern. Referenced to discussion in issue nodejs/readable-stream#102 of iojs/readable-stream
@@ -1315,6 +1315,135 @@ for examples and testing, but there are occasionally use cases where
it can come in handy as a building block for novel sorts of streams.


## Simplified API Via Revealing Constructor Pattern
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically I wouldn't call this a revealing constructor pattern, since no functionality is "revealed"; you instead just use stream.push and stream.emit("error", ...) and similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get your point, I will amend the description as well.

@Qard
Copy link
Member

Qard commented Feb 3, 2015

+1 I like it. It makes the public-facing part of streams somewhat more approachable. They're currently a bit of a nebulous thing to newbies, hence the existence of through/through2.

@sonewman
Copy link
Contributor Author

sonewman commented Feb 3, 2015

@domenic i have updated.

@Fishrock123 Fishrock123 added stream Issues and PRs related to the stream subsystem. semver-minor PRs that contain new features and should be released in the next minor version. discuss Issues opened for discussions and feedbacks. labels Feb 3, 2015

<!--type=misc-->

In simple cases there is now the added benefit of being able to construct a stream without the need of inheritance.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/the need of//

@domenic
Copy link
Contributor

domenic commented Feb 3, 2015

LGTM with minor nits. Someone more familiar with testing practices in io.js should sign off on those.

@sonewman
Copy link
Contributor Author

sonewman commented Feb 3, 2015

@domenic I appreciate your critique. I've never been that great at writing copy 😃

@Qard
Copy link
Member

Qard commented Feb 3, 2015

The console.log('ok') bits don't need to be there, and I'd refactor the writable test to use one process.on('exit', ...) and no unnecessary one/two functions.

@mafintosh
Copy link
Member

I would remove new from the examples since that is optional. Other than that LGTM from me

@domenic
Copy link
Contributor

domenic commented Feb 3, 2015

new is not optional for ES6 classes so it'd be prudent to leave them in to discourage bad habits.

@sonewman
Copy link
Contributor Author

sonewman commented Feb 3, 2015

@Qard I was trying to follow the style of the other stream tests... But I will amend.

@chrisdickinson
Copy link
Contributor

@domenic This is more for my curiosity than anything else: can you still get around that with the usual if (!(this instanceof Classname)) return new Classname(...args) in constructor(){}? If that's the case, I imagine we'll continue to take the "new is encouraged but not required" approach.

@chrisdickinson
Copy link
Contributor

Should we reframe how we're documenting this? Would it be better to integrate the existing examples with the new style of construction, and call out "old-style subclassing" as the standalone topic?

@domenic
Copy link
Contributor

domenic commented Feb 3, 2015

@chrisdickinson nope, constructors defined with class syntax have a throwing [[Call]] internal method.

@sonewman
Copy link
Contributor Author

sonewman commented Feb 3, 2015

Should we reframe how we're documenting this? Would it be better to integrate the existing examples with the new style of construction, and call out "old-style subclassing" as the standalone topic?

@chrisdickinson I thought about this very thing, as I wasn't sure where the documentation should belong. If it went before the inheritance parts then the examples were referring to documentation that was to come. By putting it after solved that problem, but one would have to read the older inheritance way to understand the use of this new simpler pattern. 😕

@chrisdickinson
Copy link
Contributor

@sonewman Cool. We should definitely revisit the docs later, but that shouldn't be considered blocking for this.

I'll leave this open until tomorrow afternoon, at which point I'll merge it if no one has objected.

@sonewman
Copy link
Contributor Author

sonewman commented Feb 4, 2015

@chrisdickinson awesome cheers dude.

@vkurchatkin
Copy link
Contributor

👍 wanted this for a long time

@sonewman
Copy link
Contributor Author

sonewman commented Feb 4, 2015

@chrisdickinson do you want me to squish this into one commit?

@Fishrock123 Fishrock123 removed the discuss Issues opened for discussions and feedbacks. label Feb 4, 2015
@Qard
Copy link
Member

Qard commented Feb 5, 2015

This test is failing for me:

=== release test-stream-writable-constructor-set-methods ===                   
Path: parallel/test-stream-writable-constructor-set-methods
assert.js:87
  throw new assert.AssertionError({
        ^
AssertionError: null == [Function: _writev]
    at process.<anonymous> (/home/sbelanger/Documents/io.js/test/parallel/test-stream-writable-constructor-set-methods.js:31:10)
    at process.emit (events.js:119:20)
Command: out/Release/iojs /home/sbelanger/Documents/io.js/test/parallel/test-stream-writable-constructor-set-methods.js
[00:43|% 100|+ 812|-   1]: Done                                                

@chrisdickinson
Copy link
Contributor

Looks like a typo. Fixing in the merge.

chrisdickinson pushed a commit that referenced this pull request Feb 5, 2015
Adds simplified constructor pattern, allowing users
to provide "read", "write", "transform", "flush", and
"writev" functions as stream options in lieu of subclassing.

Semver: minor
PR-URL: #697
Fixes: nodejs/readable-stream#102
Reviewed-By: Chris Dickinson <[email protected]>
@chrisdickinson
Copy link
Contributor

Merged in 50daee7.

Note: This is a semver-minor level change. If we don't want the next release to be semver-minor, this should be backed out first.

@sonewman sonewman deleted the simpler-stream-construction branch February 5, 2015 09:11
@sonewman
Copy link
Contributor Author

sonewman commented Feb 5, 2015

@chrisdickinson thanks for fixing the typo, I don't know how many times I ran those tests (but obviously not enough!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants