Optional PHP filtering and Jade options fix.

[icylace]

I recently discovered your Brunch plugin and find it very useful! I'm starting to use it to help with making Drupal themes so I made some changes to help with that. Hopefully these changes are also generally useful.

Here's what I did:

• Added the option to use PHP filtering.
• Added the option to change the extension used for static file output.
• Fixed Jade options not being used.

New options:

• filterPhp -- Boolean (false by default). Use the PHP custom filter.
• outputExtension -- String ('html' by default). The extension to use for the output file.

The PHP filter that's used: https://github.com/viniwrubleski/jade-php

[monokrome]

My apologies for the delayed reply here. I tried to respond on a bus, but apparently it didn't send when the request was first made. I just noticed this request after you had closed it, but don't want you to think that it was intentionally ignored.

I think that this is a very project-specific feature that belongs in a fork of the project - instead of moving up to jaded-brunch itself. However, I may consider it as an addition to the project at a later time if there is a lot of demand for the feature, or a fair specific reason for it to be included in jaded-brunch itself.

I am considering the idea of supporting a configuration variable to make the jade plugin being used a customizable option instead of requiring 'jade' specifically. Would that solve this problem in a better way?

Thanks for taking the time to leave this request, and if there are any other considerations then please feel free to respond. I will be prompt in discussion moving forward.

[icylace]

Thanks, I appreciate the response. I closed my original pull request because I wanted to redo it using a branch I created specifically meant to be pull requested.

A customizable jade plugin option doesn't seem to make sense because I'm not aware of any other jade plugin other than the official one referenced on Jade's site and also jade-php builds on top of it.

Anyway, I agree that it's probably unusual to use Brunch on PHP projects. For that reason I intend to keep my fork inclusive of that use-case.

If you do decide to merge in my pull request or something like it then chances are I'd happily retire my fork.

[monokrome]

@icylace I'm considering the ability to do the following in config.coffee in order to override the jade module being used:

exports.config =
    plugins:
        jade:
            module: 'jade-php'

This would theoretically be able to tell jaded-brunch to load a jade-php instead of the general jade module.

Would this be a simple solution that would allow your project to work as expected, or does it expose new problems?

[icylace]

Unfortunately, the effect you propose does not work because jade-php depends on jade.

[monokrome]

Are you saying that jade-php wont install jade itself?

[icylace]

jade-php does have jade as a dependency however It must be explicitly invoked. Just take a look at the example usage on its page: https://github.com/viniwrubleski/jade-php#usage

[monokrome]

@icylace I've sent a pull request on the jade-php project. It seems like an interface issue on the jade-php side. I'll update you with their responses and see how we can move forward with this.

[monokrome]

@icylace Any thoughts on this?

[icylace]

Nice. Looks like it would work.

[monokrome]

Okay. Then I can modify jaded-brunch to allow users to request a specific plugin module if that will help your use-case.

[monokrome]

@icylace I have created a branch for you to test against. The use case is that you can now set up your plugins configuration like this:

plugins:
    jaded:
        module: 'jade-php'
        extension: 'php'
        clientExtension: 'html'
        jade: # Regular jade options hare here if necessary.
            pretty: yes

It is expected that you manually install jade-php in your own project directory, and jaded will use it.

If the feature/jade-module-option branch works for you, then I will merge into master and release a new version. Thank you for helping me find this issue in jaded-brunch.

NOTE: By default, clientExtension (used when generating non-static files) will default to the value of extension. When you don't set either option, both will be 'html'. When you set extension to 'php' and don't set clientExtension, the clientExtension will be set to 'php' also. I assume that client-side files wont have PHP in them, so I have provided clientExtension as 'html' in the example.

It might make sense not to bind these two properties, so I may change this before merging. I'm going to give it some thought.

[icylace]

This is going in the right direction and would make my filterPhp option obsolete and I also just saw your note but there is something else:

My PR had an important fix for Jade options not being used. Check out that part.

[monokrome]

@icylace I have introduced that change in the branch as well. Let me know if this works for you.

[icylace]

Awesome, looks good !

I like extension and clientExtension and don't see a problem with having clientExtension, when not set, defaulting to what extension is set as.

[monokrome]

@icylace Did you verify that this works on your end?

[icylace]

When I get some time within a week or two I'll verify it.

[monokrome]

Okay. Thanks!

[icylace]

Okay, I just tried it. The installation didn't work. The "lib" folder is not created. I notice that "src/" is in .npmignore so maybe that has something to do with it? Anyway, I downloaded and compiled "index.coffee" manually.

I found a few things that might need changing. Wasn't sure if another PR was necessary because I wanted to run them by you. Here they are:

1. I've found that the declaration for localRequire should really be localRequire = (module) ->.
2. If I understand correctly one of the lines that sets up the Jade options should be @jadeOptions = _.omit options, 'staticPatterns', 'path', 'module', 'extension', 'clientExtension'
3. Also, the return statement of makeOptions() doesn't seem to need to be an assignment statement can probably just be _.extend {}, @jadeOptions, locals: data

I'm still not able to compile my Jade files, though. I know they work when compiling them directly on the command line. Will try to find out what's going on but hopefully you can shed some light on that.

[monokrome]

@icylace Thanks for recommending those changes. I have made them. I would be happy to accept pull requests, also. If you submit any, just try to do a separate request for each separated concern.

Apparently jade-php works differently than I had hoped. It monkey-patches jade instead of composing it. I have added a new option patches which allows to provide a string or array which can be a list of modules that will be required, and then @jade will be passed to them. This assumes that jade-php is using a conventional patch interface, but that's probably okay.

Let me know if this works for you. I tested it, and it did require the module and pass jade to it as expected. However, jade-php doesn't seem to be doing what is expected. Let me know if it solves the problem with your files. I think that it should, since it follows the docs.

[monokrome]

@icylace Can I get an update on your experience with this? Should I merge it or is it still a problem?

[icylace]

@monokrome Sorry, been busy but I have not forgotten! I'll test it again soon, likely tomorrow or possibly tonight. We'll see.

[icylace]

Just tried it. Unfortunately, it's still problematic. My valid Jade files won't compile. Also, the installation issue I previously mentioned is still there. What steps do you take when you do your testing? Maybe there's something there I'm missing?

[monokrome]

@icylace As described, it didn't work for me. My confusion here is that it actually calls jade-php and passes @jade to the library.

So, it seems like jade-php isn't working?

[icylace]

I actually had not gotten to the point where I wanted to try jade-php with it because it wasn't working without it. In other words, jade-php seems to be irrelevant to the problem I'm facing.

[monokrome]

@icylace Can you provide any more details about the problem? It's currently building a project of mine properly when I use npm link jaded-brunch on my linked project.

[icylace]

For sake of clarity, in my custom Brunch skeleton I currently use "jaded-brunch": "git+ssh://git@github.com:monokrome/jaded-brunch.git#feature/jade-module-option" as a dependency.

I create an empty test folder to test the installation of my Brunch skeleton. Within this new folder I use brunch new with my skeleton to setup my new Brunch project. I then follow-up with npm install. Afterwards I check the "./node_modules/jaded-brunch" folder within the project folder to find that there does not exist a "lib" or even "src" folder. This is problematic because when I use brunch build I see this error:

/usr/local/lib/node_modules/brunch/lib/watch.js:381
            throw new Error("You probably need to execute `npm install` to ins
                  ^
Error: You probably need to execute `npm install` to install brunch plugins. Error: Cannot find module '/Users/icylace/others/Sites/ramartin/_/node_modules/jaded-brunch'
    at /usr/local/lib/node_modules/brunch/lib/watch.js:381:19
    at Array.map (native)
    at loadDeps (/usr/local/lib/node_modules/brunch/lib/watch.js:366:10)
    at loadPackages (/usr/local/lib/node_modules/brunch/lib/watch.js:386:15)
    at /usr/local/lib/node_modules/brunch/lib/watch.js:403:19
    at /usr/local/lib/node_modules/brunch/lib/helpers.js:487:16
    at /usr/local/lib/node_modules/brunch/lib/helpers.js:446:14
    at /usr/local/lib/node_modules/brunch/node_modules/read-components/index.js:240:16
    at /usr/local/lib/node_modules/brunch/node_modules/read-components/index.js:54:14
    at Object.cb [as oncomplete] (fs.js:168:19)

[monokrome]

@icylace I didn't know that this was a use case. I have added a postinstall script to solve this problem. If you could try again (or link me to your brunch skeleton so that I can give it a try) then maybe that will help.

If that proves difficult or does not work, you can also build the project manually like so:

cd node_modules/jaded-brunch
npm install

[icylace]

Unfortunately, it still does not work. Here is my Brunch skeleton.

What I do is:

1. Create a new test folder and go to it.
2. brunch new gh:icylace/brunch-basis
3. npm install

[monokrome]

Thanks. I'll get it working.

[monokrome]

@icylace I am currenting moving this into master. I tested it with the commands that you tried, and it is working. However, jaded-brunch will not work properly with installing via the gh:icylace/brunch-basis format. The postinstall fails (seemingly) due to some weird directory requirement in npm. I'll look into that soon.

I will publish jaded-brunch @ 1.7.10 now, and this should work for you. Give it a try and let me know if you have issues. I'll leave this issue open until you respond with confirmation that the plugin does what you expect.

Refer to this pull request in order to see the final expected interface for configuring jaded-brunch. Please note that this will also not provide any inheritance between client-side and static extensions. (IE, client wont automatically become php if static is).

[icylace]

Thanks for the pull request.

Things seem to be working as expected aside from the npm issue you're looking into. Like I mentioned not too long ago, does "src/" being in the .npmignore file cause the issue? I suspected it did but wasn't sure. What I'll do for now is directly copy your plugin files and npm install it.

[monokrome]

@icylace The working version is published on npm, so you can just npm install jaded-brunch and get it with this feature now.

[monokrome]

@icylace Just wanting to make sure that this is working as expected?

[icylace]

Yes, it seems to be working fine now. Thanks again!