Please remove defaultLayout

[Romick2005]

Previously there were no defined defaultLayout and it was ok. But now we have: function ExpressHandlebars(config) { // Config properties with defaults. utils.assign(this, { handlebars : Handlebars, extname : '.handlebars', layoutsDir : undefined, // Default layouts directory is relative toexpress settings.view+layouts/ partialsDir : undefined, // Default partials directory is relative toexpress settings.view+partials/ defaultLayout : 'main', helpers : undefined, compilerOptions: undefined, }, config); That cause not natural behaviour in render view: `ExpressHandlebars.prototype.renderView = function (viewPath, options, callback) { ... // Pluck-out ExpressHandlebars-specific options and Handlebars-specific // rendering options. options = { cache : options.cache, view : view, layout: 'layout' in options ? options.layout : this.defaultLayout,

    data    : options.data,
    helpers : helpers,
    partials: partials,
};

this.render(viewPath, context, options)
    .then(function (body) {
        **// Here I receive good hml template body, but while 
        // (layout: 'layout' in options ? options.layout : this.defaultLayout,)
       // and defaultLayout is always specified by default to 'main' then no way to return correct body 
       //and layoutPath pointing to main which is bad behaviour because we then need manually set 
       //defaultLayout: false ()

       // Register `hbs` as our view engine using its bound `engine()` function.
       //app.engine("hbs", hbs({
       //  defaultLayout: false,
       //  layoutsDir: "views/",
       //  extname: ".hbs"
       //}));
       //app.set("view engine", "hbs");**

        var layoutPath = this._resolveLayoutPath(options.layout);

        if (### layoutPath) {
            return this.render(
                layoutPath,
                utils.assign({}, context, {body: body}),
                utils.assign({}, options, {layout: undefined})
            );
        }

        **return body;**
    }.bind(this))
    .then(utils.passValue(callback))
    .catch(utils.passError(callback));

...`

[UziTech]

couldn't you just set defaultLayout: null?

[calebolin]

couldn't you just set defaultLayout: null?

Sure, but we still need this to be fixed since it's a breaking change.

[Romick2005]

Yes I can change defaultLayout to null or false or 0 but it is not the use case for default property usage. It should cover all possible properties out of box. So what I mean that why would I specify property that I wouldn't use? If you do then please specify it explicitly.

[amypellegrini]

I'm having the same problem, but setting defaultLayout to either null or false is not fixing the issue:

Code:

app.engine('.hbs', exphbs({ extname: '.hbs', defaultLayout: false }));

app.use(morgan('dev'));
app.use(express.static('dist'));

app.set('views', 'src/templates');
app.set('view engine', 'hbs');

app.get('/', (req, res) => {
  res.render('index', {
    serverRender: ReactDOMServer.renderToString(<Home />),
    styleSheetHref: 'assets/home.css',
  });
});

Logs:

What am I missing here?

[amypellegrini]

Ok, I've fixed the issue by setting layout to false at render call:

app.get('/', (req, res) => {
  res.render('index', {
    layout: false,
    serverRender: ReactDOMServer.renderToString(<Home />),
    styleSheetHref: 'assets/home.css',
  });
});

Please make sure to release a new major version if publishing breaking changes.

[JSteunou]

I second @Romick2005

We had a hard time figuring out what's going on after upgrading (safe upgrading because no major version).

for example

res.status(403).render('403', data)

did not work anymore.

:warning: This is a breaking change :warning: and should only be published under v4

[amypellegrini]

@JSteunou @Romick2005 Yes, I'm with you on this one, I just needed to work around the issue to keep the app running, but I don't think is something I want to leave in the codebase.

That said, this raises a question around the general practice of versioning and how packages are consumed, as in my case I didn't explicitly updated anything. For now I decided to remove the ^ prefix to ensure an even more deterministic versioning of my dependencies, but still that doesn't seem to be the general practice, as the package versions were automatically added by Yarn and all of them have a ^ prefix.

In such scenario, how can I protect myself from incidents like this one? I'm quite sure that there was no ill intent in this case and some lesson has been learned by the authors/maintainers, but that doesn't prevent the same incident from happening again in the future, as we are basically trusting package publishers to follow general practice, without any guarantee that they will.

Thoughts? Opinions? All feedback is appreciated.

BTW: authoring/maintaining open source libraries can be a rough game, so kudos to authors/collaborators!

[JSteunou]

The defacto standard is semver in the JS / npm world that means

• for the author: patch means hotfixes, minor means new features, major means all delayed features that causes breaking changes
• for the consumer: use ^ (minor) or ~ (patch) prefix or none (freeze version) according to the level of trust you have to the library author and your own tests / CI.

Of course authoring libraries is tough so the duty is shared : we all need to be aware that mistake can be made and we need to test our upgrade. Even some big guys like the React team can had bugs in new release.

At our level we can alert the author and help him fix the bug either by PR, tests, feedbacks, ... then wait :)