pugjs / pug

Pug – robust, elegant, feature rich template engine for Node.js
https://pugjs.org
21.68k stars 1.95k forks source link

include mixed with filter #2929

Closed coachshea closed 6 years ago

coachshea commented 6 years ago

Pug Version: your version number here 2.0.0-rc.4

Node Version: your version number here 9.2.1

Input JavaScript Values

Input Pug

include:second-filter:first-filter file

Expected HTML

<script>code</script>
or
<style>stylesheet</style>

Actual HTML

error won't compile

Additional Comments

I have been using filters with pug and occasionally applying multiple filters with great results. I decide to utilize the include feature with my filters and noticed that within the context of include, I can no longer include multiple filters. I have tried this with several different filters and in each case, it works without include, but not with include. I don't know if this is a bug or a feature request because I am not sure of the intended response, but it made sense to me that these should be able to work together.

Thank you for your consideration.

droooney commented 6 years ago

According to this, the approach should work. I've tested this feature and it works as expected. Could you provide more info on the error?

coachshea commented 6 years ago

Thank you for the fast response. I have included below the output from an attempt to use "include:css-clean:sass file" I have gotten similar results from "include:uglify-js:livescript file" In both cases include worked with either filter one at a time and both filters worked in combination when include wasn't used. Also, a "semi-related" issue, I attempted to your jstransformer-browserify as a workaround. I figured that I could duplicate the feature with transforms and plugins from the browserify ecosystem (and of course get the benefit of using "requires" in my code) and I consistently get "browserify does not support rendering synchronously"

result from pug myfile.pug:

/usr/local/lib/node_modules/pug-cli/node_modules/pug-filters/lib/handle-filters.js:49 throw ex; ^

Error: No input specified: provide a file name or a source string to process at Object.module.exports.renderSync (/home/coachshea/documents/try-c/node-mutt/node _modules/node-sass/lib/index.js:429:11) at Object.exports.render (/home/coachshea/documents/try-c/node-mutt/node_modules/js transformer-scss/index.js:12:18) at Object.exports.render (/home/coachshea/documents/try-c/node-mutt/node_modules/js transformer-sass/index.js:24:15) at Transformer.render (/usr/local/lib/node_modules/pug-cli/node_modules/jstransform er/index.js:286:34) at filter (/usr/local/lib/node_modules/pug-cli/node_modules/pug-filters/lib/run-fil ter.js:22:30) at filterWithFallback (/usr/local/lib/node_modules/pug-cli/node_modules/pug-filters /lib/handle-filters.js:43:18) at /usr/local/lib/node_modules/pug-cli/node_modules/pug-filters/lib/handle-filters. js:31:20 at Array.forEach () at walk.includeDependencies (/usr/local/lib/node_modules/pug-cli/node_modules/pug-f ilters/lib/handle-filters.js:28:20) at walkAST (/usr/local/lib/node_modules/pug-cli/node_modules/pug-walk/index.js:23:1 8)

Thank you,

-- John Shea cell: 617-605-5763 email: coachshea@fastmail.com

On Tue, Dec 19, 2017 at 07:52:30AM -0800, droooney wrote:

According to this, the approach should work. I've tested this feature and it works as expected. Could you provide more info on the error?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.*

droooney commented 6 years ago

Yes, you are right. Include filters are applied not in reverse order but rather in the order they were written... Will do a PR today or tomorrow but don't know when it's going to be merged. Thanks for such a find that should have been detected long before :)

droooney commented 6 years ago

By the way, you don't really need clean-css or uglify-js filters. If you pass minify in your filter options (in your case sass and livescript), the filter output will get minified by pug itself using the same tools (clean-css or uglify-js).

coachshea commented 6 years ago

Thank you for your fast and helpful comments. In the meantime, it is not a big deal to me to use them in order (although for consistency your PR will certainly be a good thing). Additionally, (not sure if this should be a separate issue), any idea why the browserify transform doesn't seem to work?

Thank you again.

-- John Shea cell: 617-605-5763 email: coachshea@fastmail.com

On Tue, Dec 19, 2017 at 09:37:36AM -0800, droooney wrote:

Yes, you are right. Include filters are applied not in reverse order but rather in the order they were written... Will do a PR today or tomorrow but don't know when it's going to be merged. Thanks for such a find that should have been detected long before :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.*

droooney commented 6 years ago

Try not to change the filters order as it's clearly a nasty bug, so either wait for the new published version or better don't use the clean-css and uglify-js filters because you can get the same result like this:

// instead

style
  include:clean-css:sass style.sass
script
  include:uglify-js:livescript script.js

// write

style
  include:sass(minify) style.sass
script
  include:livescript(minify) script.js

it even looks cleaner this way, I think.

About browserify: better open another issue as it doesn't seem to be related to this one and I didn't quite get the issue, so please try to describe it a bit better there :)

coachshea commented 6 years ago

That worked perfectly. Thank you for the great advice.