Optional PHP filtering and Jade options fix. #5

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@icylace

icylace commented Feb 17, 2014

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

icylace added some commits Jan 30, 2014

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

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Feb 17, 2014

Owner

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.

Owner

monokrome commented Feb 17, 2014

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

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Feb 17, 2014

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.

icylace commented Feb 17, 2014

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

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Feb 17, 2014

Owner

@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?

Owner

monokrome commented Feb 17, 2014

@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

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Feb 17, 2014

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

icylace commented Feb 17, 2014

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

@monokrome

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Feb 21, 2014

Owner

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

Owner

monokrome commented Feb 21, 2014

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

@icylace

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Feb 21, 2014

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

icylace commented Feb 21, 2014

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 added a commit to monokrome/jade-php that referenced this pull request Feb 21, 2014

Make passing in jade optional.
This shouldn't required, and causes issues in some use cases. See monokrome/jaded-brunch#5 for an example.

@monokrome monokrome referenced this pull request in viniwrubleski/jade-php Feb 21, 2014

Merged

Make passing in jade optional. #2

@monokrome

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Feb 21, 2014

Owner

@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.

Owner

monokrome commented Feb 21, 2014

@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

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Mar 10, 2014

Owner

@icylace Any thoughts on this?

Owner

monokrome commented Mar 10, 2014

@icylace Any thoughts on this?

@icylace

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Mar 12, 2014

Nice. Looks like it would work.

icylace commented Mar 12, 2014

Nice. Looks like it would work.

@monokrome

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Mar 12, 2014

Owner

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

Owner

monokrome commented Mar 12, 2014

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

@monokrome

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Mar 12, 2014

Owner

@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.

Owner

monokrome commented Mar 12, 2014

@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 comment has been minimized.

Show comment Hide comment
@icylace

icylace Mar 12, 2014

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.

icylace commented Mar 12, 2014

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

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Mar 12, 2014

Owner

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

Owner

monokrome commented Mar 12, 2014

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

@icylace

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Mar 12, 2014

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.

icylace commented Mar 12, 2014

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

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Mar 14, 2014

Owner

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

Owner

monokrome commented Mar 14, 2014

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

@icylace

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Mar 19, 2014

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

icylace commented Mar 19, 2014

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

@monokrome

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Mar 21, 2014

Owner

Okay. Thanks!

Owner

monokrome commented Mar 21, 2014

Okay. Thanks!

@icylace

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Mar 24, 2014

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.

icylace commented Mar 24, 2014

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

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Mar 24, 2014

Owner

@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.

Owner

monokrome commented Mar 24, 2014

@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

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Apr 2, 2014

Owner

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

Owner

monokrome commented Apr 2, 2014

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

@icylace

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Apr 2, 2014

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

icylace commented Apr 2, 2014

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

@icylace

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Apr 4, 2014

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?

icylace commented Apr 4, 2014

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

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Apr 4, 2014

Owner

@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?

Owner

monokrome commented Apr 4, 2014

@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

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Apr 4, 2014

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.

icylace commented Apr 4, 2014

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

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Apr 4, 2014

Owner

@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.

Owner

monokrome commented Apr 4, 2014

@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

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Apr 8, 2014

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)

icylace commented Apr 8, 2014

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

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Apr 8, 2014

Owner

@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
Owner

monokrome commented Apr 8, 2014

@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 icylace closed this Apr 9, 2014

@icylace icylace reopened this Apr 10, 2014

@icylace

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Apr 10, 2014

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

icylace commented Apr 10, 2014

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

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Apr 10, 2014

Owner

Thanks. I'll get it working.

Owner

monokrome commented Apr 10, 2014

Thanks. I'll get it working.

@monokrome

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Apr 10, 2014

Owner

@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).

Owner

monokrome commented Apr 10, 2014

@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

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Apr 10, 2014

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.

icylace commented Apr 10, 2014

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

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Apr 10, 2014

Owner

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

Owner

monokrome commented Apr 10, 2014

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

@monokrome monokrome closed this Apr 10, 2014

@monokrome

This comment has been minimized.

Show comment Hide comment
@monokrome

monokrome Apr 14, 2014

Owner

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

Owner

monokrome commented Apr 14, 2014

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

@icylace

This comment has been minimized.

Show comment Hide comment
@icylace

icylace Apr 14, 2014

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

icylace commented Apr 14, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment