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

Make includes directory configurable #115

Merged
merged 9 commits into from
Jun 26, 2021
Merged

Make includes directory configurable #115

merged 9 commits into from
Jun 26, 2021

Conversation

valtlai
Copy link
Contributor

@valtlai valtlai commented Jun 23, 2021

This implements the possibility of configuring the includes directory proposed in #111:

  • Adds a new --includes CLI option.
  • Adds site.options.includes and a includes option for eta, nunjucks and pug.
  • Adds a options option for pug.
  • Adds site.includes(), similar to site.src().
  • Replaces the hard coded include paths with site.includes().

@oscarotero, what do you think? Do we need this?

We should also decide what should happen if the specified includes directory doesn’t start with _ (or .). Should we throw an error or just ignore the directory (like site.ignore(site.includes()))?

@valtlai valtlai force-pushed the includes branch 3 times, most recently from fd6bc68 to 4548434 Compare June 23, 2021 23:04
@oscarotero
Copy link
Member

Nice approach. One thing that I have thought about was including the possibility to configure different includes. For example, it may be more practical for some users if the css files are in /_css instead _includes/css, and the same for layouts, etc. This requires to have this option at the plugin level, not site level. For instance:

site.use(postcss({ includes: "_css" });

In fact, it's something already implemented for postcss, but not other plugins like nunjucks, eta or pug.

I think this is a more flexible solution. The plugins can have the _includes folder by default (and maybe this folder can be configured in the site, using your changes here), but with the possibility to change them.

To me, the --includes argument in CLI is a bit excesive, I can't think of a use case for that.

About the directories that doesn't start with _ or ., good point.

  • I think site.ignore(site.includes()) is an easy and clean solution.
  • But, in the other hand, the user may not want to ignore this folder for any reason:
    • Because the includes files is in a subdirectory
    • Because she want to reuse a page as a template (crazy idea)
    • Because mixes pages and layouts in the same folder, and use _ to ignore the layouts. For example:
      pages/
        page-1.md
        page-2.njk
        _layout.njk
      

So, maybe the best approach is doing nothing? Just include in the documentation a text indicating that the includes folder/files should be ignored someway.

What do you think?

- Add site.options.includes
- Add site.includes(), like site.src()
- Replace the hard coded include paths with site.includes()
@valtlai
Copy link
Contributor Author

valtlai commented Jun 24, 2021

I agree the --includes option is probably useless, so I removed it. I’m fine with doing nothing with include directories not starting with _ or ..

It’s good idea to have the includes option at the plugin level too. For example, should I

  1. pass the full options object containing both options.includes and options.options to the NunjucksEngines’s constructor

    // plugins/nunjucks.js:17
    const nunjucksEngine = new NunjucksEngine(site, options);
    
    // engines/nunjucks.js:10
    const loader = new nunjucks.FileSystemLoader(options.includes);
    this.engine = new nunjucks.Environment(loader, options.options);
  2. pass a new object containing only options.includes and options.options to the constructor

    // plugins/nunjucks.js:17
    const nunjucksEngine = new NunjucksEngine(site, { options.includes, options.options });
    
    // engines/nunjucks.js:10
    const loader = new nunjucks.FileSystemLoader(options.includes);
    this.engine = new nunjucks.Environment(loader, options.options);
  3. or add a new argument for the includes directory to the constructor?

    // plugins/nunjucks.js:17
    const nunjucksEngine = new NunjucksEngine(site, options.options, options.includes);
    
    // engines/nunjucks.js:7
    constructor(site, options, includes) {
    // :10
    const loader = new nunjucks.FileSystemLoader(includes);

@oscarotero
Copy link
Member

Probably, I'd create the nunjucks instance out of the NunjucksEngine constructor. I like the idea of having the engines as much light as possible, so they can be reused with a custom instance if you want (for example, creating your different nunjucks plugin but reusing the same engine).

This is something that I did with markdown engine: https://github.com/lumeland/lume/blob/master/engines/markdown.js (the markdown-it instance is created in the plugin file and passed to the engine in the constructor).

@valtlai
Copy link
Contributor Author

valtlai commented Jun 25, 2021

Okay, I’ll try that later, but feel free to push commit(s) here if you like.

@valtlai
Copy link
Contributor Author

valtlai commented Jun 25, 2021

Thanks for those commits!

Added a plugin-level includes option for the nunjucks and eta plugins, but not really for pug, because the include path is set in the engine instead of the plugin.

@oscarotero
Copy link
Member

Nice! I just commited a change to configure the pug engine, including the basedir (includes) from the plugin.

@oscarotero oscarotero merged commit 8462290 into master Jun 26, 2021
@oscarotero oscarotero deleted the includes branch June 26, 2021 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants