2.3.0 Creates a broken build result

[yurynix]

I'm building with the following code (the same is reproduced in cli as well):

var browserify = require('browserify');
var fs = require('fs');

var factorBundleOpts = {
    o: [ './dist/main.js', './dist/one_module.js', './dist/two_module.js', './dist/three_module.js']
};

var b = browserify();

b.add("./entry.js");
b.require('./lib/modules/one.js', { expose: "../modules/one" });
b.require('./lib/modules/two.js', { expose: "../modules/two" });
b.require('./lib/modules/three.js', { expose: "../modules/three" });

b.plugin('factor-bundle', factorBundleOpts);

b.bundle().pipe(fs.createWriteStream("./dist/modules_shared.js"));

The resulted ./dist/main.js is incomplete, the last chunk is missing, as well as the entry point (i.e. "},{}]},{},[1]);") diff The build completes with no errors =\ I'm using:

factor-bundle 2.3.0
browserify 6.1.0

I were able to "make it work", however the workaround does not make sense to me at all, if you wish to take a look at the dark magic hack, i've created a test case here:

git clone https://github.com/yurynix/browserify-bug-test
cd browserify-bug-test
npm install
./build.sh
HACK_BROWSERIFY_BUILD=yes ./build.sh

[idolize]

I'm seeing this same issue with my builds. The hack provided fixed the issue for me as well.

If I don't include the wrapper around Writeable.write() then I get the following error in my console and the app does not load correctly:

[yurynix]

The issue persists in 2.3.1 @terinjokes @substack Have an idea how I can debug this further?

[d0b1010r]

Same here. Can not use the awesome magic from this module because of this.

[terinjokes]

My entire team uses this constantly, and we've had no problems. Sorry that you're encountering this issue. I'm going to look at this today.

Edit: Fixed bad just-woke-up sentence structure and grammar.

[terinjokes]

I think I've tracked down where the problem is, and thinking about how to resolve it over lunch. Thanks for being patient.

[terinjokes]

Confirming with an upstream module the issue, will likely have a patch in tomorrow.

[yurynix]

@terinjokes Could you elaborate a bit regarding the issue? What causes it and why the dark magic of of wrapping Writeable.write fixes it?

[terinjokes]

@yurynix The monkey-patching introduces a bug, this bug happens to mask the real issue. Starting with version 2.3.0, factor-bundle's output was passed through to labeled-stream-splicer, which internally uses stream-splicer.

stream-splicer is intended to work with Transform and Duplex streams. The default behavior of factor-bundle is to output to a file, using fs.createWriteStream, which returns a writable stream. As stream-splicer is expecting a Transform or Duplex stream, the writable stream's lack of a .read method causes stream-splicer to wrap what it assumes is a v1 stream with the API of v2 streams, and implements a ._write method, clobbering the one Node.js core implements. This happens works if each chunk is small enough to be written flushed out to the kernel immediately, but starts failing miserably when a chunk is too large.

Why does monkey-patching Writable.prototype.write cause it to work? The patch has no return. Why does this matter? From the Node.js Stream documentation:

The return value indicates if you should continue writing right now. If the data had to be buffered internally, then it will return false. Otherwise, it will return true.

This return value is strictly advisory. You MAY continue to write, even if it returns false. However, writes will be buffered in memory, so it is best not to do this excessively. Instead, wait for the drain event before writing more data.

The ._write method implemented by stream-splicer checks to see if the write call returned false. If it does, it listens for the "drain" event, otherwise it calls the stream callback on a future frame. This ensures all the data gets pushed to the filesystem stream, even though it gets buffered at the end.

You can see that the broken behavior returns if you change the "HACK_BROWSERIFY_BUILD" code to implement the API properly.

if (process.env.HACK_BROWSERIFY_BUILD) {
  var Writable = require('stream').Writable;
  var oldWrite = Writable.prototype.write;

  Writable.prototype.write = function(chunk, encoding, cb) {
    return oldWrite.apply(this, arguments);
  }
}

(Aside: It's probably an implementation detail that the "fs" module uses the public-facing WritableStream interface)

I've corrected factor-bundle by changing three letters, from "ush" to "ipe". This ensures that the output of labeled-stream-splicer is piped to the WriteStream, rather than becoming part of the pipeline.

[yurynix]

@terinjokes Thanks for the through explanation! I now feel really stupid regarding forgetting the "return". I'm in no way implied that the wrapper should be used anywhere, just stumbled upon it when I were trying to debug the issue and didn't made any sense.

[terinjokes]

@yurynix I'm so sorry, this definitely wasn't the intention.

Instead, I was trying to explain both the original bug and what your monkey-patch did, as an informational response. I found the sleuthing quite fun, actually.

Hindsight is 20/20, and looking back on it now, there are definitely a better ways to phrase the initial paragraph.

[idolize]

Wow, thanks for the awesome response (and for fixing the issue)!