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

Modifies pipe to pass along highland errors #191

Closed
wants to merge 13 commits into from

Conversation

LewisJEllis
Copy link
Collaborator

Discussions on 3.0.0 have mentioned #166 a few times, so here it is on the 3.0.0 branch.

vqvu and others added 13 commits December 12, 2014 20:03
This involves:
  - Totally reimplementing stream redirection.
  - Reimplementing `consume`, `pull`, `fork`, and `resume`.

Backward incompatible;
  - The stream no longer pauses if `next` isn't synchronously called
    in
    `consume`, since this is no longer necessary to signal (lack of)
    demand to the source.
  - Lots of methods and properties that start with `_` changed
    (these
    were private anyway).
  - The callback for the stream generator and `consume` is
    guaranteed
    not to be called a second time until `next` is called.

Fixes:
  - caolan#41
  - caolan#141
  - caolan#142
  - caolan#173
- Making brace style consistently follow Stroustrup
- Cleaning up some small JSDoc blips
- Moving a function declaration to outside a loop
- A few minor things like semicolons, trailing commas and spaces, and empty blocks
@jeromew
Copy link
Collaborator

jeromew commented Jan 5, 2015

@LewisJEllis can you explain the status on this ? I understand that this would be a breaking change in 2.x so this is a '3.0.0 potential' ?

@LewisJEllis
Copy link
Collaborator Author

Yes; it's not a huge behavior change, but there definitely might be uses that this would break.

I consider it ready to merge, but there haven't been any comments on the matter since the discussion in #136, so I've left it open.

@ibash
Copy link
Contributor

ibash commented Mar 2, 2015

Anyway we can get this backported to 2? I saw the discussion in #166 but I still think it's wrong expectation and at worse awkward that connecting two highland streams via pipe does not propagate errors.

@LewisJEllis
Copy link
Collaborator Author

I agree it's silly, but simple of a change as it is, it strictly speaking is a non-BC as far as semver is concerned. I don't think we should merge it on 2.x, but there could be other options.

@ibash
Copy link
Contributor

ibash commented Mar 2, 2015

Grumble, fair enough, might make a utility to do it then.

-- 
Islam Sharabash
islam.sharabash@gmail.com
217-377-9657

On Mon, Mar 2, 2015 at 12:08 PM, Lewis J Ellis notifications@github.com
wrote:

I agree it's silly, but simple of a change as it is, it strictly speaking is a non-BC as far as semver is concerned. I don't think we should merge it on 2.x, but there could be other options.

Reply to this email directly or view it on GitHub:
#191 (comment)

@svozza
Copy link
Collaborator

svozza commented Mar 2, 2015

I agree it's not ideal but with the fix gone into through there's really no reason to ever use pipe to join two Highland streams.

@ibash
Copy link
Contributor

ibash commented Mar 2, 2015

@svozza use case is I want to pass a stream to a function and have it write to it.
e.g.

stream = _() ...;

writeDataToStream(stream);

Inside of writeDataToStream it needs to do something like:

function writeDataToStream(stream) {
  stream2 = ...;

  stream2.pipe(stream);
}

Is there a better way to do that?

@vqvu
Copy link
Collaborator

vqvu commented Mar 3, 2015

The point of streams is that you can construct the stream objects synchronously even if the data source isn't. What specifically requires you to ever write a writeDataToStream function?

Why can't you do this?

function getDataAsStream() {
    var stream2 = ...;
    return stream2;
}

// Same ... used in your example above to construct the stream
var stream = getDataAsStream() ...;

Edit: Re-ordered sentences for clarity.

@ibash
Copy link
Contributor

ibash commented Mar 3, 2015

You're right -- should be able to do that. Was just thinking about it as:

  1. Build a pipeline
  2. Push data down the pipeline

But it makes just as much sense as

  1. Get your datasource
  2. Build a pipeline

@vqvu
Copy link
Collaborator

vqvu commented Mar 4, 2015

OK. Since we have #240 now, and we seem to have consensus, I'll go ahead and close this as unnecessary.

@vqvu vqvu closed this Mar 4, 2015
@LewisJEllis LewisJEllis mentioned this pull request Jul 28, 2015
34 tasks
@LewisJEllis
Copy link
Collaborator Author

@vqvu you originally closed this as unnecessary thanks to #240, and I just trusted your judgment, but @svozza still wants it as per recent discussion on #179. I'm not sure what the situation is but maybe you guys could discuss whether this is necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants