-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
nice! ill leave some comments inline |
// Grab our Jade templates. | ||
if (!pkg.config.templates) return callback(); | ||
var files = pkg.config.templates.filter(filterJade); | ||
function templates (plain) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this an options
object instead, for future compatibility in case we want to add more. and then change the name from plain
to string
, so it would be like this. and then also add the "templates"
key as the first argument to match the native concat
and copy
plugins
with all that it would end up looking like this:
builder.use(jade('templates', { string: true }));
awesome, happy to merge with those changes applied. would be sweet to get tests in this PR as well /cc @Swatinem to check it out, it'll remove jade dep |
Nice! And I must shamefully admit, I haven’t tried the new builder yet. Will it still work on the commandline via |
yeah I will update the readme and add tests tomorrow if I get to it :) The cli isn't compatible with the newest builder just yet. It shouldn't be hard to implement it again though. |
I added tests now, updated the docs and changed the code based on your suggestions. Can you review again? :) |
, jade = require('component-jade'); | ||
|
||
var Builder = require('component-builder'); | ||
var write = require('fs').readFileSync; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small typo
looks good to me! thanks for the awesome pull. i'll fix that typo |
hmm, the templates won't actually work if you don't maybe add a note in the readme? |
Yeah, I guess the problem right now is in the builder: https://github.com/component/builder.js/blob/master/lib/plugins/commonjs.js#L62 Everything that is not json or a js file will get stringified and exported. That means this var jade = require(\'jade-runtime\');module.exports = function template(locals) {... gets turned into this module.exports = "var jade = require(\'jade-runtime\');module.exports = function template(locals) {..." even though there is already a |
yeah. we need an equivalent of the old |
this messed me up too -- couldn't figure out what was going wrong until I saw that the whole thing was stringified. |
If #169 gets merged we can use that. In the meantime I'll just use the old builder |
I've basically rewrote the module so that it works with @yields changes on the newest builder (0.12.0).
If you would merge this the basic usage will be:
component-jade will compile to plain html if you set
opts.plain
to trueOtherwise it would compile to templates like it did before.
Also, jade and the runtime.js are now not included in the module itself, instead they must be added as dependencies.
If you don't like any of those changes you can simply ignore this PR :) I will add tests and update the readme as soon as it gets accepted.