rotundasoftware / parcelify

Add css to your npm modules consumed with browserify.
MIT License
251 stars 20 forks source link

Upgrading browserify #12

Closed bowd closed 9 years ago

bowd commented 10 years ago

Hi there, I've been playing around with parcelify recently and was wondering what would it take to update it's support to the latest browserify version.

I've started looking into that a bit and basically I guess it's about getting parcel-map to play with the new browserify API, and I'm playing around with refactoring that to get the tests passing. But before I dive deeper into that I'm curious what's stopping you guys for pursing this... I'm sure you have a much clearer vision and have already thought about this.

For the record I haven't felt a really pressing need to update it so far (tho' I'm still not done porting everything from component), but it just feels a bit weird (3.44.0 vs 5.11.0).

But I really like the initiative, and I think this general vision is the way to do it, so I'd be happy to help if I can.

cowwoc commented 9 years ago

@dgbeck This issue is a big deal for me too. Is it possible for you to update to the latest version of Browserify?

ghost commented 9 years ago

After the code is updated to the latest version it might be good to remove the require('browserify') altogether from the index.js and encourage people to pass in the browserify bundle object instead or install the package as a plugin. This way the end-users can decide what version of browserify to use and parcelify won't need to keep updating for browserify version updates.

cowwoc commented 9 years ago

Related to what @substack just wrote, I don't (yet) understand why parcelify needs to run browserify under the hood. I was hoping to use browserify to process JS files and parcelify to process the remaining asset types (e.g. CSS, images).

Meaning, I don't really want parcelify to create any JS bundles for me... browserify already seems to do a good job there and I think we will end up with a more flexible API (for bundling JS files) if this is left outside of parcelify.

dgbeck commented 9 years ago

So, couple issues on this thread

  1. Bumping the browserify version would be great. I did venture to do this upgrade at one point but ran into some problems. If memory serves some extra events were being fired and / or some were missing, and I thought it the problem was somewhere in module-deps but was not able to isolate. I would like to see us keep current with browserify, however. @substack (good to hear from you - it has been too long!) maybe I'll give this another shot and run it by you if I run into problems. I would also like to explore if parcelify could be wrapped up as a browserify plugin.
  2. Related to #20, @cowwoc you can definitely use the js bundle from browserify and just use parcelify for css (or other) bundles. Will comment over there on why parcelify can optionally build a js bundle.
dgbeck commented 9 years ago

@substack sure do miss you when you're not here! I'm trying to bump the browserify version... looks like you have been busy. Couple things I've ran into so far:

  1. Looks like the package event has been renamed to file. ok
  2. Looks like some things have changed in the arguments to the dep event. ids are now numbers instead of file paths. ok. But I'm not seeing how to build the dependency graph anymore, because the deps property no longer contains absolute paths.

For example, here main.js depends on node_modules/my-module/index.js:

{
  entry: true,
  expose: false,
  basedir: undefined,
  file: '/Users/david_beck/Documents/git/parcelify/test/page1/main.js',
  id: 2,
  order: 0,
  source: 'require( \'my-module\' );\n\nconsole.log(\'page 1!!!\');\n',
  deps: { 'my-module': 1 },
  index: 2,
  indexDeps: { 'my-module': 1 }
}
{
  id: 1,
  source: 'console.log( "in my-module" );',
  deps: {},
  file: '/Users/david_beck/Documents/git/parcelify/test/node_modules/my-module/index.js',
  index: 1,
  indexDeps: {}
}

How can we deduce that /Users/david_beck/Documents/git/parcelify/test/page1/main.js depends on /Users/david_beck/Documents/git/parcelify/test/node_modules/my-module/index.js from this info?

ghost commented 9 years ago
  1. The package event is still around. There were some issues with the package event not always firing in ~8 that have been fixed in the latest version 9 releases.
  2. The ids are integers by the time the dep event fires, but before the label phase of the pipeline the ids are still full system paths: https://github.com/substack/browserify-handbook#label You can also set opts.fullPaths which is what watchify uses.

Now might be a good opportunity to switch parcelify over to be a plugin. Plugins just take a bundle as their first argument and an options hash as their second argument. Here's the basic structure for a slimmed down version of parcelify that just reads style fields without the per-parcel output splitting implemented as a plugin:

var glob = require('glob');
var fs = require('fs');
var path = require('path');
var spawn = require('child_process').spawn;
var duplexer = require('duplexer');

module.exports = function (b, opts) {
    if (!opts) opts = {};
    var out = opts.output || opts.o;
    var queue = [];

    var output = reset();
    b.on('reset', function () { output = reset() });

    if (!output) {
        return b.emit('error', new Error(
            'glob-css output must be a string filename or a stream'
        ));
    }

    b.on('package', function (pkg) {
        if (pkg.style) {
            pending ++;
            glob(pkg.style, { cwd: pkg.__dirname }, function (err, files) {
                pending --;
                queue.push.apply(queue, files.map(function (p) {
                    return fs.createReadStream(path.resolve(pkg.__dirname, p));
                }));
                enqueue();
            });
        }
    });

    b.pipeline.on('end', function () { finished = true });

    var queueing = false;
    var pending = 1;

    function enqueue () {
        if (queueing) return;
        if (queue.length === 0 && pending === 0) {
            return output.end();
        }
        if (queue.length === 0) return;
        queueing = true;
        var q = queue.shift();
        q.pipe(output, { end: false });
        q.once('end', function () {
            queueing = false;
            enqueue();
        });
    }

    function reset () {
        queue = [];
        queueing = false;
        if (typeof out === 'string') return fs.createWriteStream(out);
        if (typeof out === 'function') return out();
        if (typeof out === 'object' && out.pipe) return out;
    }
};

This approach is nice because the browserify instance is passed in, so the consumer can control the browserify version used, so long as it's compatible (and all of the API endpoints used here are documented, tested, and not going to change any time soon). To run this plugin.js file, just do:

$ browserify -p [ ./plugin.js -o public/bundle.css ] main.js -o public/bundle.js

which will build public/bundle.css with all of the contents from package.json style fields and public/bundle.js with the javascript output.

dgbeck commented 9 years ago

@substack thanks for this example code and the answers above. Very helpful. Working on this project now and would estimate I'm about 50% of the way through it. However getting caught up on something that I think is either very simple or impossible.. hoping you can tell me quickly which is the case.

How can parcelify determine when a particular phase of the pipeline is DONE. For example, it needs to know when the label phase is finished (all rows have been passed through the pipeline) so that it can build a complete dependency graph.

Also, what does each row represent in the wrap phase of the pipeline? That is,

b.pipeline.get( 'wrap' ).push( through.obj( function( row, enc, next ) {
   console.log( row );  // what does this buffer represent?
} ) );
ribeiroct commented 9 years ago

Thanks for the great work on this.

Since i needed the latest browserify version, I ended up using substack simplified version(as a plugin).

It works great, the only thing i'm missing from this approach is the minification process. I've been trying to do it but without any success. Any tips ? Thanks for the help.

EDIT:

Over simplified example: watchify -d -p [ css_plugin.js -o bundle.css ] *.jsx -o bundle.js catw -c 'uglifycss $FILE' bundle.css -o bundle.min.css -v

i wrapped both commands on package.json scripts, and run them like "npm run watchify & npm run catw"

Maybe i did go on the long stupid way, but it works. lol If anyone knows the proper way to add this to the plugin please say so.

dgbeck commented 9 years ago

Thanks @ribeiroct. Good news.. the change to a browserify plugin is in the bump-browserify branch, passing all tests, and watch seems to be working as well. Still needs to be documented and put through some more tests but hoping to merge to master by the end of the week.

dgbeck commented 9 years ago

k this is done and done. parcelify v1.0.0 just published to npm, compatible with most recent version of browserify and works as a standard plugin. Thanks @ribeiroct @cowwoc et. al for pushing this along, and to @substack for his help.