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

helpers: introduce Stream() #31

Closed
wants to merge 4 commits into from
Closed

helpers: introduce Stream() #31

wants to merge 4 commits into from

Conversation

Fishrock123
Copy link
Owner

Refs: #26

This turned out quite reasonable, I think. I might just like it. (It was pretty easy to implement, worked first try??)

@Raynos
Copy link
Collaborator

Raynos commented Jun 7, 2019

Are you sure you want to implement a thenable ?

What about adding a method that returns a promise like stream.wait()

@Fishrock123
Copy link
Owner Author

Ok, a little more complexity and this is now capable of this kind of thing:

  const streamSource = new Stream(fileSource, zlibTransform)
  const streamSink = new Stream(passThrough, fileSink)
  const stream = new Stream(streamSource, new PassThrough(), streamSink)

  await stream.start()

In which Streams are now also a partial passthough for conditional complexity reduction...

I also tried to get rid of bindSource and bindSink but it didn't go too well. Relatedly, I think that bindCb should move to start() (and or start should be thenable or return a promise).


@Raynos Is there anything wrong with it being thenable? I guess start could just return a promise, if it's bad...

@Fishrock123
Copy link
Owner Author

PR for the start(exitCb) thing: #32

@Raynos
Copy link
Collaborator

Raynos commented Jun 9, 2019

Is there anything wrong with it being thenable?

I'm not sure if it impacts debugging. For example, a thenable will not play well with unhandledRejections as it's invisible. However your implementation does throw the error if no-one is waiting on it with then()

The other issue with thenable is that someone might refactor

await stream

To

stream.catch(handleError)

And then be confused

@Fishrock123
Copy link
Owner Author

I think that start() should maybe just take a callback and if none is provided return a promise. Should solve those concerns and make things simpler and more sensical.

@Raynos
Copy link
Collaborator

Raynos commented Jun 10, 2019

👍 for adding to start() thanks.

@Fishrock123
Copy link
Owner Author

Simplified a lot.

Also stuff like Stream(Stream(Stream(...), ...), ...) now works.

Copy link
Collaborator

@Raynos Raynos left a comment

Choose a reason for hiding this comment

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

This looks nice to me.

sink = null

constructor (...components) {
const last = components.length - 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider checking for components.length >= 2 here with some kind of assertion.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It could do more typechecking, but for now that will just make things needlessly more work to read.

Is there an issue with doing Stream(<single component>) though? Maybe there's a use case for that. But also I suppose we could only expand to that later if necessary.

tests/stream-stream-async.js Outdated Show resolved Hide resolved
Fishrock123 added a commit that referenced this pull request Jun 11, 2019
Introduces `Stream()`, a much more user friendly way of composing
streaming operations out of bob components.

`Stream()`s also act as a bare passthrough to their subcomponents and
can be use as a component when composing higher-order `Stream()`s.

Relys on the newly added `start(exitCb)` to keep things nice.

Refs: #26
PR-URL: #31
@Fishrock123
Copy link
Owner Author

landed as 5a460b4

@Fishrock123
Copy link
Owner Author

To be clear, await new Stream(streamSource, new PassThrough(), streamSink).start() doesn't quite work today. I've left the Promise returning out for simplicity for now, but the same is achievable by:

const stream = new Stream(streamSource, new PassThrough(), streamSink)
await util.promisify(stream.start.bind(stream))

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.

None yet

3 participants