configuration `layoutsDir` & `partialsDir` is invalid

[JaylanChen]

After 3.0.1 was released, the configuration options layoutsDir and partialsDir became invalid.

utils.assign(this, {
        handlebars     : Handlebars,
        extname        : '.handlebars',
        layoutsDir     : 'views/layouts/',
        partialsDir    : 'views/partials/',
        defaultLayout  : undefined,
        helpers        : undefined,
        compilerOptions: undefined,
}, config);

Because after Fixes #147, the custom configuration options layoutsDir and partialsDir will be overridden.

    var viewsPath = options.settings && options.settings.views;
    if (viewsPath) {
        view = this._getTemplateName(path.relative(viewsPath, viewPath));
        this.partialsDir = path.join(viewsPath, 'partials/');
        this.layoutsDir = path.join(viewsPath, 'layouts/');
    }

I hope it can be repaired as soon as possible.Thank you.

[dogbutcat]

Yeah, I also found it. It is needed when developer not defined the layoutsDir and partialsDir as given default config path here is relative https://github.com/ericf/express-handlebars/blob/5d27bb50ba299d7ba185dfc5b9f8057bf7129256/lib/express-handlebars.js#L24-L32

so I think maybe change like this is fine for global custom one or default one

    utils.assign(this, {
        handlebars     : Handlebars,
        extname        : '.handlebars',
       // layoutsDir     : 'views/layouts/',
       // partialsDir    : 'views/partials/',
        defaultLayout  : undefined,
        helpers        : undefined,
        compilerOptions: undefined,
    }, config);

and

    if (viewsPath) {
        view = this._getTemplateName(path.relative(viewsPath, viewPath));
        this.partialsDir =  this.partialsDir || path.join(viewsPath, 'partials/');
        this.layoutsDir = this.layoutsDir || path.join(viewsPath, 'layouts/');
    }

I changed on my local like this and it work fine with my old settings

[jfoclpf]

@dogbutcat

we don't need to comment here:

    utils.assign(this, {
        handlebars     : Handlebars,
        extname        : '.handlebars',
       // layoutsDir     : 'views/layouts/',
       // partialsDir    : 'views/partials/',
        defaultLayout  : undefined,
        helpers        : undefined,
        compilerOptions: undefined,
    }, config);

because the function ExpressHandlebars.prototype.getPartials later on will get the partialDir array defined by the constructor. Those values are just the default in case we don't provide partialsDir.

The problem really resides here:

    // Express provides `settings.views` which is the path to the views dir that
    // the developer set on the Express app. When this value exists, it's used
    // to compute the view's name. Layouts and Partials directories are relative
    // to `settings.view` path
    var view;
    var viewsPath = options.settings && options.settings.views;
    if (viewsPath) {
        view = this._getTemplateName(path.relative(viewsPath, viewPath));
        this.partialsDir = path.join(viewsPath, 'partials/');
        this.layoutsDir = path.join(viewsPath, 'layouts/');
    }

Why the partialsDir array should be overwritten just by the fact that a viewsPath is defined?

We should not in any case if viewsPath is truthy, ignore the partials/ directory as seen from viewsPath. Considering that partialsDir must always be an Array, I propose

    if (viewsPath) {
        view = this._getTemplateName(path.relative(viewsPath, viewPath));

        if(Array.isArray(this.partialsDir)) {
          this.partialsDir.push(path.join(viewsPath, 'partials/'));
        } else {
          this.partialsDir = [path.join(viewsPath, 'partials/')];
        }

        if(Array.isArray(this.layoutsDir)) {
          this.layoutsDir.push(path.join(viewsPath, 'layouts/'));
        } else {
          this.layoutsDir = [path.join(viewsPath, 'layouts/')];
        }
    }

Maybe it could be more compact, but I think it keeps the structure. I tested and it works

[dogbutcat]

@jfoclpf Thanks for replying.

First, why I need to comment out the definition in constructor is to deal with the pass-through settings defined outside, eg. exphbs({layoutsDir:'./views'}). So when executor goes to ones below, it can check whether developers have already defined it or not, if negative, it will create default one, because settings.views will always exist. It is provided by Express framework with application's view path, detailed in Express's source code.

        this.partialsDir =  this.partialsDir || path.join(viewsPath, 'partials/');
        this.layoutsDir = this.layoutsDir || path.join(viewsPath, 'layouts/');

Second, for your provided modification, why you MUST push this path path.join(viewsPath, 'partials/') to this.partialsDir and why I can't defined by my own? This plugin has exposed the entire variable, you can write any path(es) you need and also checked in further usage.

At end, I don't know why you think this.layoutsDir is to be an Array. You can also find this property's description here: https://github.com/ericf/express-handlebars#layoutsdirviewslayouts .

[jfoclpf]

@dogbutcat Ok, you have the personal experience, but the constructor does

var utils = require('./utils');

function ExpressHandlebars(config) {
    // Config properties with defaults.
    utils.assign(this, {
        handlebars     : Handlebars,
        extname        : '.handlebars',
        layoutsDir     : 'views/layouts/',
        partialsDir    : 'views/partials/',
        defaultLayout  : undefined,
        helpers        : undefined,
        compilerOptions: undefined,
    }, config);

utils come from the other file utils.js

exports.assign    = Object.assign || require('object.assign');

which I suppose it clones the object, and thus if you defined your partials at config exphbs({layoutsDir:'./views'}), it should theoretically overwrite the default immediately (I suppose)

Second, for your provided modification, why you MUST push this path path.join(viewsPath, 'partials/') to this.partialsDir and why I can't defined by my own?

I'm not saying you can't, I am just trying to make the module as fully compatible as possible, and the folder partials/ seems to be the default one, and there's no harm to keep it there as a safeguard in case viewsPath is truthy

At end, I don't know why you think this.layoutsDir is to be an Array. You can also find this property's description here: https://github.com/ericf/express-handlebars#layoutsdirviewslayouts .

I know, but the internal function getPartials at line 51 of the that file, the 1st thing it does is to convert it to an array

var partialsDirs = Array.isArray(this.partialsDir) ?  this.partialsDir : [this.partialsDir];

because it may be defined as an array:

var hbs = exphbs.create({
  extname: '.hbs',
  partialsDir: [
    path.join(__dirname, 'views', 'common'),
    path.join(__dirname, 'views', 'main'),
    path.join(__dirname, 'css', 'merged-min'),
    path.join(__dirname, 'tables')
  ],
  helpers: hbsHelpers
})

Therefore I assumed that probably it's better to always considered it internally as an array. But you're right, maybe it's superfluous.

[jfoclpf]

@ericf Could you kindly take a quick look into this, because I suppose many modules that depend on this one, like mine, are not working?

just replace this

    if (viewsPath) {
        view = this._getTemplateName(path.relative(viewsPath, viewPath));
        this.partialsDir = path.join(viewsPath, 'partials/');
        this.layoutsDir = path.join(viewsPath, 'layouts/');
    }

by either this

    if (viewsPath) {
        view = this._getTemplateName(path.relative(viewsPath, viewPath));
        this.partialsDir =  this.partialsDir || path.join(viewsPath, 'partials/');
        this.layoutsDir = this.layoutsDir || path.join(viewsPath, 'layouts/');
    }

or this

    if (viewsPath) {
        view = this._getTemplateName(path.relative(viewsPath, viewPath));

        if(Array.isArray(this.partialsDir)) {
          this.partialsDir.push(path.join(viewsPath, 'partials/'));
        } else {
          this.partialsDir = [this.partialsDir || path.join(viewsPath, 'partials/')];
        }

        if(Array.isArray(this.layoutsDir)) {
          this.layoutsDir.push(path.join(viewsPath, 'layouts/'));
        } else {
          this.layoutsDir = [this.partialsDir || path.join(viewsPath, 'layouts/')];
        }
    }

Thank you

[sahat]

Thank you for reporting the issue. The bug has been fixed and the new version v3.0.2 is now published on npm.