Support for specifying browserify transforms from a command line #17

Closed
andreypopp opened this Issue Jun 13, 2014 · 40 comments

7 participants

@andreypopp

My workflow has changed recently, before I was publishing code on npm with unprocessed sources (using JSX + ES6) but now I decided it is easier to publish ES5 compiled code instead so Node.js could consume it w/o troubles.

So I went and removed all browserify config from package.json. But that made testing a bit more difficult because I need to run tests against compiled code which is almost impossible to do right with --watch (I need to watch code to compile it and watch with mochify to run tests — so complex!).

Instead I want to propose adding transforms to mochify via command line -t/--transform option. What do you think?

@mantoni
Owner

Agreed. The goal should even be to support all browserify options that make sense.

@andreypopp andreypopp added a commit to andreypopp/mochify.js that referenced this issue Jun 13, 2014
@andreypopp andreypopp Reuse browserify option parser to create browserify instance
(closes #17)
b854b5d
@andreypopp andreypopp added a commit to andreypopp/mochify.js that referenced this issue Jun 13, 2014
@andreypopp andreypopp Reuse browserify option parser to create browserify instance
(closes #17)
83009d6
@krues8dr

Going back to what I was trying to do with this ticket, it would be nice if all of the browserify options could be passed through mochify - I'm in need of support for both -t coffeeify as well as --extension .coffee.

@mantoni
Owner

Yes. Agreed. I already refactored the arguments parsing into lib/args.js and made it easier to extend.
One of you guys interested in adding options?

@krues8dr

I'm a little out of my depth on this one, not being super-familiar with the internals of browserify. I'm worried that we'll have to reimplement the entire of browserify's args.js here : https://github.com/mantoni/mochify.js/blob/master/bin/cmd.js#L80-L105 Maybe we just call browserify's args.js and use that to instantiate browserify? Then we can use that instantiation to bolt on the things needed here.

Also, maybe how about using something like subarg as they use in browserify to have a whole set of subargs that are automatically passed to browserify, rather than having to parse them separately? e.g. mochify --browserify [ --extension .coffee -t coffeeify ] ?

@mantoni
Owner

I like the sub args idea for --browserify, but would that work with nested sub args?

@krues8dr

According to the subargs docs, it should ? I haven't tried it though. ;)

@mantoni
Owner

Sounds like the cleanest solution and should also be straight forward to implement.

Alternatively, the idea of a mochify.opts or a .mochify config file already came up. Browserify args could be configured there as well.

@andreypopp

Yeah, --browserify [ ... ] would be the best solution, I think, we can reuse browserify/bin/args.js.

@krues8dr

Well, I got a little ways down the path of refactoring the existing args.js using subarg and got it working, only to realize too late that browserify doesn't offer any hooks into its args parsing, it expects a string only. As a result, using a quoted string for the browserify arguments looks like the easiest way to deal with this since it's handled by the native argument passing, e.g. mochify --browserify "--extension .coffee -t coffeeify"

Here's the adapted version of args.js using subarg, in case anyone wants to think about refactoring that code for some other reason. https://gist.github.com/krues8dr/e27b5015868bd86b6ddb

@mantoni
Owner

Did you look into @andreypopp's take on reusing the option parser?
#18

@krues8dr

Yup, that's what I'm copying now! :)

@krues8dr

Ok, so I copied what @andreypopp did for #18 here: krues8dr@e141199 but now I'm just getting an error similar to #7:

TypeError: 'undefined' is not an object
    at http://localhost:58572?brout/js/bundle:20
    at http://localhost:58572?brout/js/bundle:14680
    at http://localhost:58572?brout/js/bundle:14533
    at http://localhost:58572?brout/js/bundle:18087
    at http://localhost:58572?brout/js/bundle:16223
    at http://localhost:58572?brout/js/bundle:20566
    at http://localhost:58572?brout/js/bundle:17367
    at http://localhost:58572?brout/js/bundle:15629
TypeError: 'undefined' is not an object
2014-07-02 16:07:16.926 phantomjs[585:507] CoreText performance note: Client called CTFontCreateWithName() using name "Times New Roman" and got font with PostScript name "TimesNewRomanPSMT". For best performance, only use PostScript names when calling this API.
2014-07-02 16:07:16.926 phantomjs[585:507] CoreText performance note: Set a breakpoint on CTFontLogSuboptimalRequest to debug.

At this point, PhantomJS is running and everything is just hanging out, but it doesn't appear to be doing anything. I'm running this command: ./node_modules/mochify/bin/cmd.js --browserify "--extension .coffee -t coffeeify" ../http2/test/spec/annotator_spec.coffee, which gives the proper output if I just pass those same arguments to browserify. I'm a bit out of my depth on the internals here as to what could be generating that error via brout?

@mantoni
Owner

Uh, that's a bug in phantomic. It generates the stack trace URL wrong. Should be http://localhost:58572/js/bundle:20.

I quickly fixed it in phantomic 0.8.2 which is already part of todays mochify release if you pull and re-install dependencies. From looking at your diff, I suppose you will have to rebase and fix conflicts, sorry.

What you will get from the fix is that the stack trace is translated to the original sources using source maps. You will see where the error is actually thrown.

@krues8dr

Great, that was a pretty clean merge. But although I now know where the error is actually being thrown, I'm no closer to knowing why it's being thrown.

TypeError: 'undefined' is not an object
    at ../http2/node_modules/backbone-events-standalone/backbone-events-standalone.js:19
    at node_modules/brout/lib/brout.js:35
    at node_modules/mocaccino/lib/setup-browser.js:36
TypeError: 'undefined' is not an object

That line is still in the comments of backbone-events-standalone.js so something is clearly still being wonky.

@mantoni
Owner

Yes, that looks wonky. Maybe it has to do with the relative path (../http2/...)? Is that a linked module?

@krues8dr

It's from the main project I'm using this in. Symlinking in my build of mochify and calling it without that path results in the same error. ./node_modules/mochify/bin/cmd.js --browserify "--extension .coffee -t coffeeify" ./test/spec/annotator_spec.coffee

@mantoni
Owner

Hm. Maybe the coffee script translation screws up the source maps?

@barneycarroll

Aside from implementation bugs, should this be the way to go for all peer dependencies? Seeing as I'm already using mochify -U tdd -R xunit, I unconsciously assumed I could pass through the rest of my Mocha arguments (case in point: --recursive).

So the inclination leads towards:

$ mochify -U tdd -R xunit --mocha[ "--recursive" ]

…but then this is full of assumptions too:
1. Should I be quoting the Mocha args, if that's an implementation detail specific to Browserify's API?
2. -U and -R are lifted from the Mocha API, but are these being passed straight through (ie without Mochify performing logic and talking to Phantom)?
3. Should subargs represent the totality of the invocation for that peer dependency? Otherwise there could be conflicts with the args passed to Mochify directly…

Just want to get the expectations clear in my head before diving into implementation.

@mantoni
Owner

No, this is not the way forward, simply because it does not make a particularly nice API and also because not all the arguments make sense to be passed to mocha (e.g. --debug or -w since it need different handling in the Mochify context). The same is true for browserify. I've started to work on this on a branch and already converted the arguments parsing to use subargs, but mainly to allow to pass arguments to custom transforms:

$ mochify --transform [ foo -x -y ]

In case of --recursive I'd prefer it to be passed down just like -U and -R are.

@ferlores

👍. I need to run mochify with https://github.com/thlorenz/proxyquireify in order to stub dependencies. What are the next steps? Can I help somehow?

@mantoni
Owner

@ferlores You can help by verifying that the --transform support in the just released Mochify 1.3.0 actually works :)

@mantoni
Owner

Released Mochify 1.4.0 with --plugin support. Closing issue since --transforms now work since 1.3.0.

@barneycarroll Please file a separate issue or send a PR for --recursive

@mantoni mantoni closed this Oct 17, 2014
@ferlores

awesome, thanks!

@tobi-or-not

Can someone give me an example how to pass options to a transform? I am trying to use stringify but I am failing to pass the correct options.
In another place in my gulpfile, I do this:
browserify().transform(stringify(['.ejs']))

The stringify documentation show this example:

stringify({
  extensions: ['.txt', '.html'],
  minify: true,
  minifier: {
    extensions: ['.html'],
    options: {
      // html-minifier options
    }
  }
})

Thus I am assuming something along the lines of the following approach should work but I does not. What is the correct way to do this?

gulp.task('test', function(){
  mochify('./test/**/*.js', { transform: ['stringify', {'extensions': '.ejs'}]});   
});
@mantoni
Owner

@tobi-or-not That's a good point. The current implementation is built to work nice from the command line, but programatic use could / should be improved.

This should work for you:

gulp.task('test', function(){
  mochify('./test/**/*.js', { transform: [{ _: 'stringify', 'extensions': '.ejs' }]});
});

So the config and the transform name are defined in one object with _ defining the transform name.

If you'd like a nicer API, a pull request is very welcome :)

@tobi-or-not

Thanks @mantoni for your quick response!

Unfortunately, the above example dos not work and throws the following error:

TypeError: Object stringify has no method 'shift'
    at […]/node_modules/mochify/lib/mochify.js:143:28
    at Array.forEach (native)
    at module.exports ([…]/node_modules/mochify/lib/mochify.js:138:31)
    at Gulp.<anonymous> ([…]/gulpfile.js:92:3)
    at module.exports ([…]/node_modules/gulp/node_modules/orchestrator/lib/runTask.js:34:7)
    at Gulp.Orchestrator._runTask ([…]/node_modules/gulp/node_modules/orchestrator/index.js:273:3)
    at Gulp.Orchestrator._runStep ([…]/node_modules/gulp/node_modules/orchestrator/index.js:214:10)
    at Gulp.Orchestrator.start ([…]/node_modules/gulp/node_modules/orchestrator/index.js:134:8)
    at /usr/local/lib/node_modules/gulp/bin/gulp.js:121:20
    at process._tickCallback (node.js:419:13)

I tried to get the format right but failed miserably ...

Any idea? What is the format on the CLI that should work?

@mantoni
Owner

Sorry, I got the example wrong. Try this:

gulp.task('test', function(){
  mochify('./test/**/*.js', { transform: [{ _: ['stringify'], 'extensions': '.ejs' }]});
});

So it just got even more ugly. Ouch.

@tobi-or-not

Ah. Cool. Now, at least, I get the same error when I call it from CLI and Gulp.

[…]/node_modules/mochify/node_modules/browserify/node_modules/readable-stream/lib/_stream_readable.js:516
  dest.on('unpipe', onunpipe);
       ^
TypeError: Object function browserifyTransform(file) {
    console.log('in transform');
    if (!hasStringifiableExtension(file, extensions)) {
      console.log('in strf ext');
      return through();
    }
    var chunks = [];

    var write = function (buffer) {
      chunks.push(buffer);
    };

    var end = function () {
      var contents = Buffer.concat(chunks).toString('utf8');

      this.queue(stringify(minify(file, contents, options)));
      this.queue(null);
    };

    return through(write, end);
  } has no method 'on'
    at Transform.Readable.pipe ([…]/node_modules/mochify/node_modules/browserify/node_modules/readable-stream/lib/_stream_readable.js:516:8)
    at wrapTransform ([…]/node_modules/mochify/node_modules/browserify/node_modules/module-deps/index.js:467:11)
    at […]/node_modules/mochify/node_modules/browserify/node_modules/module-deps/index.js:204:26
    at nr ([…]/node_modules/mochify/node_modules/browserify/node_modules/module-deps/index.js:233:13)
    at […]/node_modules/mochify/node_modules/browserify/node_modules/resolve/lib/async.js:48:21
    at […]/node_modules/mochify/node_modules/browserify/node_modules/resolve/lib/async.js:127:35
    at […]/node_modules/mochify/node_modules/browserify/node_modules/resolve/lib/async.js:99:39
    at […]/node_modules/mochify/node_modules/browserify/node_modules/resolve/lib/async.js:65:30
    at […]/node_modules/mochify/node_modules/browserify/node_modules/resolve/lib/async.js:23:18
    at Object.oncomplete (fs.js:107:15)

I am pretty sure that this one has nothing to do with mochify but I am still stuck on it. Have you gotten stringify to work with mochify in this way before?

@tobi-or-not

A little update. Instead of passing the stringify transform to mochify, I changed https://github.com/mantoni/mochify.js/blob/master/lib/mochify.js#L197 to the following

var stringify   = require('stringify');
b.transform(stringify(['.ejs']));
b.bundle(callback);

With this change in place, mochify runs perfectly from gulp. (I have not tested CLI). So my guess is that something about calling stringify from mochify is causing the problem

@mantoni
Owner

Mochify needs proper support for this in the API, where you can require the transform yourself.

Something like this:

mochify({
  transforms: [myTransform1, myTransform2]
});

@tobi-or-not Would you care to look into this? There is a transform under /test/fixture that can be used for a test case.

@tobi-or-not

I will. I am not sure though if I'll be able to wrap my head around it since I am pretty new to the node side of JS.

@ferlores

I'm facing the same kind of issues trying to use different transforms. I was looking a bit into the code and since the only thing that mochify does is to create a browserify pipe, maybe is a good idea to return the browserify instance and let the user deal with the transforms on their own.

I'm thinking to change line https://github.com/mantoni/mochify.js/blob/master/lib/mochify.js#L215

b.bundle(callback);

for

return b;

And let the user call bundle(). Of course this changes the API of mochify, but moving forward I think that this is the right approach. @mantoni what do you think?

@mantoni
Owner

@ferlores I think I like this solution. Make it a PR! I would release this together with a bunch of other changes in 2.0.0.

@ferlores

I've created #52 but I need your help to fix some tests. You will find more info in the PR. Thanks

@jfalameda

Hi,

Is this issue already fixed? I don't know if it is related to Browserify or Mochify, but it is worth trying. This is how I am initializing Mochify:

var mochify = require('mochify');
var stringify = require('stringify');

var test = process.argv[2];

mochify(__dirname+"/"+test+'/*.js', {
    reporter : 'tap',
    phantomjs: 'node_modules/.bin/phantomjs',
    cover    : true
})
    .transform(stringify(['.hbs']))
    .bundle();

When executing the code it still interpret HBS files as javascipt so it throws a Error: Unexpected token <

Any help would be appreciated. I also tried to setup stringify form the command line with no success.

Thanks!

@mantoni
Owner

This issue is fixed since you can now work directly with the browserify instance (as you do in your example). I don't know stringify, so I'm of little help here. But I would try without the cover option, to make sure that it's not a coverify thing.

@tobi-or-not

Not sure if this helps (my setup is a little different) but when I add 'watch: true' to my configuration, I get the same error, so it looks like this is where the problem comes from.

@ferlores

@jfalameda There is a issue in coverify with the non.js files, I've sent a PR but it seems substack is not actively maintaining this project anymore. That's why I've created mochify-istanbul. You can see a simple use case in the README of this repo here

@jfalameda

@mantoni Thanks! You are right, it was actually wrong with stringify. I used stringify({extensions: ['.hbs]}) instead and it worked. For some reason .transform(stringify(['.hbs']))works when making a direct call to a bundle created with browserify() but not with the bundle obj returned by mochify. Maybe it has to do with the browserify version mochify uses.

This was referenced Jan 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment