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

Convert to streams2 readable streams using through2() #646

Closed
wants to merge 1 commit into from

Conversation

parshap
Copy link
Contributor

@parshap parshap commented Feb 10, 2014

Summary

This patch converts all through streams created using through to be streams2 streams using through2. I ran into an issue where I expected the stream returned by bundle() to buffer data until I read from it (steams2 semantics).

Example

var fs = require("fs");
var browserify = require("browserify");
var output = require("stream-stream")();

output.write(fs.createReadStream("static.js", { encoding: "utf8" });
output.write(browserify("client.js").bundle());
output.end();
output.pipe(fs.createWriteStream("bundle.js"));

The bundle stream emits its data immediately before the stream-stream is ready to read from it. I am interested in hearing if there is a better approach I should be using to concatenate static js to my browserify bundle.

Note: This is broken nondeterministically, only when static.js is sufficiently large.

Details

Tests passed as-is except for test/standalone_events.js. This is because the test is expecting a close event and it seems like streams2 readable streams do not emit one by default nor do they propagate close events from piped streams. I changed this test to note expect a close event.

Many tests use through() and I did not change those - I moved through to devDependencies.

Things to Review

  • Usage of objectMode in the right places
  • Does anything rely on streams1 semantics (e.g., "auto flowing")? This is kind of an backwards-incompatible API change. I added calls to stream.resume() to automatically start flowing where it made sense (e.g., when there is a callback given).
  • Should a close event be emitted?

@ghost
Copy link

ghost commented Feb 12, 2014

It might good to bump the major just in case for this one and not worry too much about it.

@ghost
Copy link

ghost commented Mar 28, 2014

Merged in 3.37.0. None of the tests broke so I just bumped the minor.

@ghost ghost closed this Mar 28, 2014
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant