Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Process templating syntax in content files as well #35

Closed
ismay opened this issue Nov 1, 2014 · 17 comments
Closed

Process templating syntax in content files as well #35

ismay opened this issue Nov 1, 2014 · 17 comments

Comments

@ismay
Copy link

ismay commented Nov 1, 2014

I've seen a couple of issues and pull requests that all relate to processing the templating language in content files, in one way or another: #21, #24, #22, #27, #26.

I'm opening this issue to focus discussion on what I think is actually at the core of these issues and prs, to hopefully reach a solution more quickly and eliminate any confusion. Let me briefly summarize (and reiterate what I said in #21):

  • People want to nest templates (or 'extend' them)
  • People want to process content files as templates (like with inPlace), but still want to apply the template as defined in the front-matter (which inPlace does not do)

Extending

As I said in #21 (and as others have said as well), extending templates is more a functionality of the templating language, and does not have to be added to metalsmith-templates (in my opinion). It's already possible.

Process contents

So that leaves the second bulletpoint: processing the templating language in content files, whilst still applying the template as defined in the files front-matter. I think solving that would basically directly or indirectly address all issues and prs mentioned above.

So what would be the best solution to address that? A new option, or modify inPlace (if this corresponds to what it was originally meant to do)? @ianstormtaylor , what do you think?

@ianstormtaylor
Copy link
Contributor

Actually, talked about something similar with @treygriffith a while back about this plugin technically should have been made as two plugins. One for the idea of "layouts", probably metalsmith-layouts and one solely for the purpose of in-place templating, probably metalsmith-templates. I think that would solve a lot of the confusion around the different options?

@ismay
Copy link
Author

ismay commented Nov 2, 2014

Yeah I think it would! I think that would be great. @mayo, this would work for you as well right?

@ismay
Copy link
Author

ismay commented Nov 3, 2014

@ianstormtaylor, if you're game for this as a solution, is there anything I can do to help? I'm not on your level as a coder, but maybe there's something I can do?

@treygriffith
Copy link

@ismay I haven't touched this code in a while, but I think to split this into two plugins would just require copying everything over into both new repos, and changing lines 73-79 to only support one option from the conditional for each plugin.

Just dividing this plugin into two separate plugins with a clear purpose / use case would be a huge step in the right direction, and probably focus some of the conversations - it should become pretty clear which of the two new plugins should be responsible for which proposed features.

@garygreen
Copy link

Personally, I would dislike it if it was split into two plugins. I think it would cause more confusion. A lot of the issues spawn from using consolidate, which imo is a pretty hacky way of attempting to integrate all the possible templating engines. Sometimes it's impossible to realistically have the same API for all of them; some precompile templates, have template inheritance, or the possibility of adding filters or global functions.

With that said, there has to be a better way of supporting a simple templating engine plus the ability to 'tap into' the underlying engine instance (by means of e.g. a onInit callback) to add custom templating options.

@ismay
Copy link
Author

ismay commented Nov 4, 2014

@garygreen, I guess there's a tradeoff. On the one hand having a single plugin that processes templates makes sense, to avoid duplication of otherwise pretty similar code. But on the other hand it also makes sense to me to separate it into two because we're talking about two different tasks:

  • Task 1: For each file from the files object, take the template as defined in the file's front-matter and dump the file verbatim in that template's contents variable.
  • Task 2: For each file from the files object, process any templating syntax in the file itself.

I could go either way. A simple option for metalsmith-templates that toggles task 2 would work just as well for me. So long as there is a way to toggle both task 1 and task 2 separately (because right now inPlace enables task 2, but disables task 1).

With that said, there has to be a better way of supporting a simple templating engine plus the ability to 'tap into' the underlying engine instance (by means of e.g. a onInit callback) to add custom templating options.

I'd say that this isn't about passing options to the templating engine, but more about metalsmith-template's processing logic. Tapping into the underlying templating engine would be nice, but is a different issue as I see it. But I agree, that would be nice.

@ianstormtaylor , @treygriffith : given what garygreen's said, what's your preference: still want to separate the functionality or keep it in a single plugin? Just so I know what to works towards.

@garygreen
Copy link

@ismay yes sorry, I was going a bit off topic! Probably another issue (more to do with consolidate.js) which is why I have to initialize my own nunjucks-template plugin.

Hmm actually on second thoughts I can see why this plugin should be split up into two...that's IF it's to remove the template inheritance support with the template yaml field. The reason being is that this plugin attempts to add template inheritance when you might already be using a templating engine that natively supports template inheritance like swig or nunjucks. That causes a big conflict when you want to do 'inPlace' because how do you tell your templating engine to use that as the template to extend/inherit from?

Therefore I would like to see:

  1. A generic template plugin that simply renders the contents in each source file, passing to the templating engine WITH the yaml matter in the top of the file.
  2. A 'layout' plugin that has support for the template yaml which will pass the rendered content from the template onto the main layout. Again, this would conflict if you was using something like swig or nunjucks so that would have to be noted when using the plugin. It's really just a plugin for templating engines that don't support template inheritance.

In the instance of using just swig/nunjucks, you would use number 1 and define the template you want to use in your actual source file (or just none at all), instead of in the yaml matter:

src/home.html

---
title: Homepage
---

{% extends 'layouts.main' %}

{% block content %}

    blah

{% endblock %}

layouts/main.html

<html>
   <title>{{ title }}</title>
   {% block content %}{% endblock %}
</html>

Essentially your using your source files as full views, with the ability to add yaml matter that automatically get's sent to your renderer as include data. That would be awesome and clear up a lot of confusion I think.

@ismay
Copy link
Author

ismay commented Nov 4, 2014

@garygreen, If you want to do that you can just set inPlace: true. That'll do exactly what you're describing. This issue is more about separating these two tasks:

  1. processing templating syntax in source files
  2. applying a template to source files (via the front-matter)

As right now, inPlace switches between one or the other, but does not allow both. What you want to do is just 1, which is already possible. What I want to do is 1 & 2, which isn't possible. Hence the solution to separate these two pieces of functionality, to make that possible as well.

@ismay
Copy link
Author

ismay commented Nov 4, 2014

Ok, so these two repos are working examples of what we've been talking about:

I did some testing with both plugins, and combining them works just fine (tested with .use(templates).use(layouts), not the other way around). Does in-place templating and applies the template selected in the front-matter to the result.

What does everyone think? Proceed with this? Because then we'd need a concensus on the names (layouts and templates for example) and one new repo, to allow me to create a pull-request for both plugins.

Also, if this is what we'll do then the new metalsmith-templates (in-place templating) will need some further changes:

  • Is the directory option still needed for in-place templating? I'm using handlebars, which requires me to attach everything to the Handlebars object directly, so I don't think I'd have to use these. Not sure about other languages.
  • The default option can be removed
  • The checks for the template can be removed (from the function on line 46)

@ianstormtaylor
Copy link
Contributor

Those look cool to me! Would you be interested in maintaining them yourself (ie. not transferring back)? Totally fine with me if you want to go that route as well.

@ismay
Copy link
Author

ismay commented Nov 10, 2014

You mean just maintaining them as alternatives next to the canonical metalsmith-templates?

@ismay
Copy link
Author

ismay commented Nov 13, 2014

Because then maybe it would be good to rename ismay/metalsmith-templates to prevent confusion about what it does, and remove the ismay/metalsmith-layouts repo, since it'll be similar to segmentio/metalsmith-templates (with inPlace:false)?

To me the main goal is an option for in-place templating that's independent of the application of (layout) templates to the files in src. Anything that allows that is fine by me. And I'd be happy to maintain a plugin myself, let me know what you'd prefer!

@lygaret
Copy link

lygaret commented Nov 15, 2014

Out of curiousity, is there a reason it wouldn't work to just invoke the template plugin twice, once with inPlace: true and once without? That's what I've been doing (via API of course) and it's working rather well for me.

@ismay
Copy link
Author

ismay commented Nov 16, 2014

@lygaret, Ha, yeah that's a lot easier :). Only possible improvement on this would be putting this in a single plugin with two independent flags in my opinion. What do you think @ianstormtaylor, shall I remove my versions of metalsmith-templates since this works just as well?

@ianstormtaylor
Copy link
Contributor

Oh sorry, I totally would have suggested that solution, I use that too. I
think your two versions are still useful though, because this plugin is
trying to do too much. Would be nicer if it had been split up into two
plugins from the start IMO.

On Sun, Nov 16, 2014 at 6:02 AM, Ismay notifications@github.com wrote:

@lygaret https://github.com/lygaret, Ha, yeah that's a lot easier :).
Only possible improvement on this would be putting this in a single plugin
with two independent flags in my opinion. What do you think
@ianstormtaylor https://github.com/ianstormtaylor, shall I remove my
versions of metalsmith-templates since this works just as well?


Reply to this email directly or view it on GitHub
#35 (comment)
.

@ismay
Copy link
Author

ismay commented Nov 18, 2014

@ianstormtaylor, Ok, I've made some final changes and they're both ready for use. Quick recap:

https://github.com/ismay/metalsmith-templates @ 1.0.0

  • render files in-place by default
  • remove default, directory and inPlace
npm install git://github.com/ismay/metalsmith-templates.git

https://github.com/ismay/metalsmith-layouts @ 1.0.0

  • remove inPlace option
  • use layout instead of template in the front-matter to specify a layout
  • set default folder for layouts to layouts instead of templates
npm install git://github.com/ismay/metalsmith-layouts.git

I might have made a mistake or two, but as far as I can tell everything works just fine (worked fine when I tested them). Only thing that I haven't updated yet are the tests. As far as I'm concerned this issue is resolved, but please feel free to reopen if I've forgotten anything!

@ismay
Copy link
Author

ismay commented Nov 22, 2014

For posterity:

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

No branches or pull requests

5 participants