resume bundleStream after attaching cache hooks

[olegskl]

When callback is provided to the original bundle, the 'end' event is never fired. Proposal to add a .resume() to the stream.

[olegskl]

Just to add a little bit of context: I'm trying to use browserify-incremental with karma-browserify which passes a callback to the .bundle function. When browserify sees the callback it does:

if (cb) {
  output.on('error', cb);
  output.pipe(concat(function (body) {
    cb(null, body);
  }));
}

And this messes up the attachCacheHooksToPipeline function in BrowserifyCache.js. I'm not very good with streams, but having .resume() seems to make it work.

[Macil]

I would like to reproduce the issue you're having and make a test for it, but I can't reproduce it. I don't run into any problems when passing a callback to the bundle method.

package.json

{
  "name": "foo",
  "version": "1.0.0",
  "dependencies": {
    "browserify": "^13.0.0",
    "browserify-incremental": "^3.1.1"
  },
  "license": "ISC"
}

test.js

var path = require('path');

var browserify = require('browserify-incremental'),
  b = browserify({cacheFile: path.join(__dirname, 'browserify-cache.json')})
    .require('./foo.js');

b.bundle(function() {
  console.log('bundle callback');
});

foo.js

console.log('foo');

$ node test.js 
bundle callback
$ cat browserify-cache.json 
{"modules":{"/private/tmp/foo/foo.js":{"file":"/private/tmp/foo/foo.js","source":"console.log('foo');\n","deps":{}}},"mtimes":{"/private/tmp/foo/foo.js":1459458561000},"dependentFiles":{}}

[olegskl]

Indeed it works... I don't understand. Must be something to do with karma-browserify then. I'll need to take a closer look tomorrow morning.

[olegskl]

What if instead of './foo.js' you require some big library (or several big libraries), like React? When I do it, the bundle callback is not called.

[olegskl]

Bump.
Do you need additional information?

[Macil]

I haven't been able to reproduce the issue. Could you make an example repository and give the commands necessary to reproduce it?

[olegskl]

Here it is: https://github.com/olegskl/test-browserify-incremental

[olegskl]

I have asked a friend to test it on Windows. Same issue. Perhaps it's an asynchronicity problem, because it works well on small files.

[Macil]

Thanks for the reproduction case! (Note: you should use npm-shrinkwrap for this sort of thing.) I can verify it fails for me.

Before merging this, I would prefer to have a test for it, and to investigate whether this might actually be a Browserify bug. I haven't looked into it yet after getting this reproduction case but I'm not sure how browserify-cache-api could be doing something differently when React is imported.

[olegskl]

Sorry for the late reply, vacation time ^_^'.
I've spent a couple of days figuring out the reason why it wasn't working.

this might actually be a Browserify bug

Unlikely, because running the same code using Browserify works.

not sure how browserify-cache-api could be doing something differently when React is imported

Any dependency that requires a lot of files (like react, crypto, or browserify itself) will result in failure of calling the bundle callback.

By default streams are created with highWaterMark option set to 16 (items, not bytes, as all streams there are in "object mode"). Every required file writes to stream and progressively fills the internal buffer. Because the outputStream is not in "flowing" mode, then once the internal buffer is filled, it seems to get into "paused" mode and never gets to the "end" event.

[william26]

+1

[D34THWINGS]

I investigated this bug with @olegskl. To complete his answer, when the stream created by browserify gets paused because of output stream buffer overflow, chunks will no longer leave the internal buffer of browserify stream (more details here). Because of this, the concat stream that should execute the callback will no longer receive any event and therefore can't call the callback on finish. Also the buffer of output stream that you create is 2 times the size of the highWaterMark because it's a Duplex stream. Both read stream and write stream have a buffer and it will fill both of them before pausing the browserify stream. So any library under 32 files will trigger the callback, otherwise it will stay silent

[olegskl]

Bump @agentme @jsdf. Do you require anything else to merge this?

[VanCoding]

I'm also dealing with this bug. Would be cool if we got the fix :)

[VanCoding]

I can also confirm that this fix works btw.