API enhancements

[jgoz]

This improves the plugin API use case for factor-bundle.

• Re-hook into the pipeline on reset — necessary when calling bundle() multiple times, e.g., with the watchify API
• Attach factored method to browserify instance — creates/returns a through stream through which all factored bundle streams will pass. Definitely open to suggestions on suitability, naming, etc.
• Add opts.outputs alias for opts.o to mirror opts.entries
• In addition to strings & streams, accept functions that return strings/streams in opts.o. This is necessary to recreate streams for multiple bundle() calls. Works nicely with lazypipe.
• Added documentation around plugin usage with browserify API. The example uses gulp, but I'd be happy to change that.

[ghost]

• re-hooking on reset seems good
• attaching a factored method is pretty weird and creates side-effect ordering issues. We already have ways of exposing the underlying streams.
• aliases seem fine
• functions for opts.o is a good idea, especially when using with watchify
• I would rather stick to vanilla js and command-line examples only in the documentation.

[jgoz]

@substack Awesome, thanks for the feedback. I'll clean it up.

[jgoz]

We already have ways of exposing the underlying streams.

What did you have in mind here? It's easy to extract streams from opts.o before they get passed in, but when you have functions that return streams, there isn't really a way to collect them after the stream event.

[ghost]

Could we just have a new event that emits an event emitter that emits each stream? That would fit in well with the existing api.

[terinjokes]

I see we've been duplicating work over the past day. :)

What I was considering doing was exposing a pipeline instance for each stream, so that someone downstream could make changes to wrapping and packing. Does this help?

[jgoz]

@terinjokes That's a really good idea. I can give you push access to my fork if you want.

[terinjokes]

This works for me.

[jgoz]

@terinjokes I rebased & overwrote the branch to get rid of the factored() stuff. Your turn! :)

[terinjokes]

Gracias, getting on that.

[terinjokes]

@jgoz Sorry about the delay. I've been busy with my real work stuff 😦. I'm going to try to get to this tomorrow (Saturday).

[jgoz]

@terinjokes No problem at all! This isn't blocking my real work stuff - just a nice to have. If you need help let me know!

[jgoz]

@terinjokes Do you think it would be worth merging this as-is and splitting the pipeline-per-stream feature into a separate request? That way at least it works with watchify, even if the API isn't ideal.

[terinjokes]

I'm just hesitant on having four different possible settings for a single thing. Overloaded APIs are also confusing APIs. Let me think about it tonight, and I'll either do a follow up comment or a merge tomorrow.

[jgoz]

I'm just hesitant on having four different possible settings for a single thing.

You mean for opts.o? That's a fair point. If you removed the "function that returns a string/stream" option, it would still work with the watchify command and for direct file outputs. You just wouldn't be able to reuse stream outputs, but that could be addressed in the pipeline work.

[terinjokes]

Function returning a stream is what I had locally to resolve the watchify issue, I'm not sure of that usefulness in the current state of a function returning a string is (I can't think of any new information in this case).

That said, I primarily wanted to think about how the pipeline version would work, and jot it down either here or another ticket, before moving forward here. Would hate for this to shoot that in the foot.

[jgoz]

I'm not sure of that usefulness in the current state of a function returning a string is

It's probably not useful at all - really just a consequence of accepting stream/string/function as possible values and not restricting the function output.

That said, I primarily wanted to think about how the pipeline version would work, and jot it down either here or another ticket, before moving forward here. Would hate for this to shoot that in the foot.

Sounds good.

[jgoz]

What about this:

Every output stream emits a factor.pipeline event via the browserify instance with 2 parameters: stream id and pipeline. The pipeline has 2 phases: pack and wrap (not sure if anything else really applies).

This would allow you to listen for the event and hook the pipeline as required. You also wouldn't have to pass anything for o/outputs.

var source = require('vinyl-source-stream');
var buffer = require('vinyl-buffer');
// etc.

function wrap(id) {
  return source(path.basename(id))
    .pipe(buffer())
    .pipe(sourcemap.init({ loadMaps: true }))
    .pipe(uglify())
    .pipe(sourcemap.write())
    .pipe(gulp.dest('out'));
}

b.on('factor.pipeline', function (id, pipeline) {
  pipeline.get('wrap').push(wrap(id));
});
b.plugin('factor-bundle');
b.bundle().pipe(wrap('common.js'));

Advantages of this approach:

1. The pipelines would get recreated with every reset, emitting the factor.pipeline event and re-hooking the streams each time, so it's watchify-compatible.
2. Minimal pollution of browserify instance - just using it as a namespaced event relay.
3. Could be kept compatible with opts.o - if specified, it would just hook into the pipeline internally and pipe to the stream/file.

[jgoz]

@terinjokes Have a look at jgoz#1 and jgoz#2 — both create pipelines for the output bundles but they're hooked differently (1 uses events, 2 uses a callback param). I prefer the callback myself.

Any thoughts on either of these approaches?

[terinjokes]

I prefer #1, trivial example aside.

[jgoz]

OK. Is it worth tacking that on to this PR and cleaning up as needed?

[terinjokes]

@jgoz Probably worth it. Might be good to squash them into two commits though.

[jgoz]

Will do

[jgoz]

@terinjokes Squashed + added docs.

[sevein]

@jgoz and @terinjokes, this is looking great!
Any chance to merge this at some point? Thanks.

[terinjokes]

Thanks. I think it looks good too, but I want to take another look when I'm not sick. 😷

[sevein]

@jgoz, I tried both branches and they both worked great for me. I don't have strong preferences, so up to you guys! What I've noticed is that when I switched to your branches I had to start using absolute paths in my entries list because relative paths stopped working. Namely, they are completely ignored, though a error is emitted when they can't be found. Weird!

[jgoz]

@sevein

What I've noticed is that when I switched to your branches I had to start using absolute paths in my entries list because relative paths stopped working

That's probably because my branches haven't been rebased since 20e00a2 was committed.

[sevein]

oh @jgoz, I didn't notice that!

[terinjokes]

@jgoz I'm going to rebase this and merge, assuming you have no other changes.

[jgoz]

@terinjokes No other changes from me!

[sevein]

Sweet!