nunjucks deprecated render function

gillesgw commented 5 years ago

Hi,

I'm using consolidate with express and nunjucks : I need for some pretty simple cases to configure my own instance of nunjucks, for adding custom filters among other things.

I can see a way in the doc, but it doesn't work, I have a "engine.configure is not an option" error, as describe in #244 , marked as resolved.

I digged a little "exports.nunjuncks.render" in lib/consolidate.js, and maybe I'm missing something, but if I comment quite almost the function, it seems to works perfectly :

in my app :

const cons = require('consolidate'); const nunjucks = require('nunjucks'); cons.requires.nunjucks = nunjucks.configure(); // cons.requires.nunjucks.addFilter('foo') ... app.engine('njk', cons.nunjucks);

and in consolidate : /lib/consolidate.js :

`exports.nunjucks.render = function(str, options, cb) { return promisify(cb, function(cb) {

try {

  var engine = options.nunjucksEnv || requires.nunjucks || (requires.nunjucks = require('nunjucks'));

  var env = engine;
  //
  // // deprecated fallback support for express
  // // <https://github.com/tj/consolidate.js/pull/152>
  // // <https://github.com/tj/consolidate.js/pull/224>
  // if (options.settings && options.settings.views) {
  //   env = engine.configure(options.settings.views);
  // } else if (options.nunjucks && options.nunjucks.configure) {
  //   env = engine.configure.apply(engine, options.nunjucks.configure);
  // }
  //
  // //
  // // because `renderString` does not initiate loaders
  // // we must manually create a loader for it based off
  // // either `options.settings.views` or `options.nunjucks` or `options.nunjucks.root`
  // //
  // // <https://github.com/mozilla/nunjucks/issues/730>
  // // <https://github.com/crocodilejs/node-email-templates/issues/182>
  // //
  //
  // // so instead we simply check if we passed a custom loader
  // // otherwise we create a simple file based loader
  // if (options.loader) {
  //   env = new engine.Environment(options.loader);
  // } else if (options.settings && options.settings.views) {
  //   env = new engine.Environment(
  //     new engine.FileSystemLoader(options.settings.views)
  //   );
  // } else if (options.nunjucks && options.nunjucks.loader) {
  //   if (typeof options.nunjucks.loader === 'string') {
  //     env = new engine.Environment(new engine.FileSystemLoader(options.nunjucks.loader));
  //   } else {
  //     env = new engine.Environment(
  //       new engine.FileSystemLoader(
  //         options.nunjucks.loader[0],
  //         options.nunjucks.loader[1]
  //       )
  //     );
  //   }
  // }

  env.renderString(str, options, cb);
} catch (err) {
  throw cb(err);
}

}); };`

Is there a reason to keep this code, and if so, maybe we need to adapt it ? I didn't digged enough and I'm new to consolidate and nunjucks, but I can try to investigate.

doowb commented 5 years ago

Hi @gillesgw, I didn't really understand why that code was there when I first saw it either. After looking at the issue you linked, then following it to the PR that added the code, I noticed there was a new note added to the README.md giving a hint at what to do.

It suggests passing the configured environment on nunjucksEnv and don't pass a settings.views. After typing that, I realized that since you're using express, I'm not exactly sure how to do that.

If you have the time and could dig into nunjucks a little more to see if there is a way to detect the difference between a normal nunjucks instance and a configured instance (i.e result of nunjucks.configure()), that would be awesome!

I think this would be the immediate solution, but I'm also thinking of ways to refactor consolidate to make it easier and more intuitive to pass your own configured engine instance.

gillesgw commented 5 years ago

Hi @doowb, thanks for your answer!

It was my first thought too, but the project I'm in is a took over project and in my refactoring, I'm switching from ejs to nunjuncks : so for now, I have old pages working with ejs and new pages with nunjuncks, thanks to consolidate. Thing is, settings.views is used by ejs views...

That made me thinking that this could be improved anyway, because shouldn't we be able to have a custom nunjuncks instance AND override settings.view if needed ?

Your idea to detect if it's a brand new nunjucks instance or an "env" configured instance seems to be the way to go for me. I'll try to see that as soon as I can!

titanism commented 1 year ago

We have forked this repository for maintenance and released it under @ladjs/consolidate, see https://github.com/ladjs/consolidate.js. We have merged PR's and updated it for email-templates. Please click the "Watch" button to get notified of all releases at https://github.com/ladjs/consolidate.js. Thank you 🙏

PR welcome at the new repo once new release is published today!

tj / consolidate.js

nunjucks deprecated render function #313