jadeify works in dev mode but not production

[johnsoftek]

If I follow the suggestion to use jadeify (http://ampersandjs.com/learn/templates), it works with isDev: true but not isDev: false.

I have tried adding the transform to package.json:

  "browserify": {
    "transform": ["jadeify"]
  },

and in Moonboots config:

        browserify: {
            debug: false,
            transforms: ['jadeify']
        },

Both methods give the same error for production mode:

stream.js:94
      throw er; // Unhandled stream error in pipe.
            ^
Error: Parsing file /Users/john/AppDev/amp1/client/vm/collectionDemo/collectionDemo.jade: Line 2: Unexpected identifier
    at parseDeps (/Users/john/AppDev/amp1/node_modules/moonboots-express/node_modules/moonboots/node_modules/module-deps/index.js:256:45)
    at done (/Users/john/AppDev/amp1/node_modules/moonboots-express/node_modules/moonboots/node_modules/module-deps/index.js:236:13)
    at applyTransforms (/Users/john/AppDev/amp1/node_modules/moonboots-express/node_modules/moonboots/node_modules/module-deps/index.js:206:41)
    at /Users/john/AppDev/amp1/node_modules/moonboots-express/node_modules/moonboots/node_modules/module-deps/index.js:178:17
    at fs.js:271:14
    at Object.oncomplete (fs.js:107:15)

It seems that module-deps/index.js is trying to trace dependencies for an untransformed .jade file. If I modify module-deps/index.js to skip .jade files in the same way that it skips .json files, the problem is avoided. But there must be a better fix in Moonboots - or maybe I have not configured browserify/jadeify correctly?

UPDATE

I tried this fix in moonboots/index.js. Change

        mdeps(self.config.main, {
            resolve: hashBundle._resolve.bind(hashBundle)
        })

to this

        var opts = {
            resolve: hashBundle._resolve.bind(hashBundle)
        }
        if (self.config.browserify.transforms) {
            opts.transform = self.config.browserify.transforms
        }
        mdeps(self.config.main, opts)

It works.

[lukekarrys]

Thanks for the issue! And it looks like you found a fix. Would you be willing to make that change in a fork and file a pull request for it?

And oddly enough, I wasn't able to reproduce in this simple test case I created, but your update makes sense, since module-deps would definitely need access to the transforms as well.

[johnsoftek]

Okay. I'll file a pull request.

I reproduced the error in your test case. Just change template.jade to:

 p All that you require to crash

[lukekarrys]

Yep, I can confirm the crash in that case. Thanks for your work investigating this!

[johnsoftek]

I made the change but cannot get 100% test coverage - just one line. See https://github.com/johnsoftek/moonboots/tree/fix-issue-55

[lukekarrys]

If you make this line https://github.com/johnsoftek/moonboots/blob/fix-issue-55/index.js#L376-L378 test for self.config.browserify.transforms.length then the coverage passes.

It seems like under the hood browserify is modifying that object to default transforms to [] so that line always passes which makes it fail 100% coverage.

[johnsoftek]

That relies on browserify setting self.config.browserify.transforms. Oh well...

On 18 October 2014 07:44, Luke Karrys notifications@github.com wrote:

If you make this line https://github.com/johnsoftek/moonboots/blob/fix-issue-55/index.js#L376-L378 test for self.config.browserify.transforms.length then the coverage passes.

It seems like under the hood browserify is modifying that object to default transforms to [] so that line always passes which makes it fail 100% coverage.

— Reply to this email directly or view it on GitHub https://github.com/HenrikJoreteg/moonboots/issues/55#issuecomment-59580705 .

[lukekarrys]

Agreed, that's not ideal especially since its internal to browserify and we have no guarantee that patch releases wont change that.

The other option is to make a copy of browserify.transforms before passing it to browserify and use that? It'd be nice to keep coverage at 100%.

[johnsoftek]

I have submitted a pull request but I think your suggestion is an improvement. I will work on it further.

[latentflip]

@lukekarrys is this pull request ready to go now? Just spent a long time rediscovering this bug :(

[lukekarrys]

Sorry @latentflip. It's pretty much ready, we were mostly just bikeshedding how to get it to 100% test coverage. I +1 this being merged if you need it.

[latentflip]

Cool, I think the current PR is at 100% now so +1 on getting this released.

Philip Roberts

On 30 Oct 2014, at 22:32, Luke Karrys notifications@github.com wrote:

Sorry @latentflip. It's pretty much ready, we were mostly just bikeshedding how to get it to 100% test coverage. I +1 this being merged if you need it.

— Reply to this email directly or view it on GitHub.

[lukekarrys]

@latentflip do you want to merge and release? if not, I can get to it tomorrow

[latentflip]

Cool I can release it tomorrow morning.

Philip Roberts

On 30 Oct 2014, at 22:50, Luke Karrys notifications@github.com wrote:

@latentflip do you want to merge and release? if not, I can get to it tomorrow

— Reply to this email directly or view it on GitHub.