Skip to content

Add "view options" settings for rendering engines #3613

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nvlled
Copy link

@nvlled nvlled commented Apr 2, 2018

Avoids cluttering app.locals, and is consistent
with the assignment of the "view engine" settings:

app.set("view options", {
    basedir: "somedir"
});
app.set("view engine", "pug");

The intent is also stated more clearly in comparison to:

app.locals.basedir = "somedir"

Avoids cluttering app.locals, and is consistent
with the assignment of the "view engine" settings.

app.set("view options", {
    basedir: "somedir"
});
app.set("view engine", "pug");

The intent is also stated more clearly in comparison to:

app.locals.basedir = "somedir"
@dougwilson
Copy link
Contributor

Seems reasonable. Would this only apply to the set default view engine, or to all view engines (and if so, how would conflicting options be resolved)? Please add some tests when you find the time 👍

@nvlled
Copy link
Author

nvlled commented Apr 2, 2018

Yeah, it applies to all view engines, although I've only looked and modified the app.render code, so I may not be entirely sure. It's basically an alias for the app.locals. I'll do a double check.

Edit: Yeah, it applies to all view engines, not just the default one.
It merges the options in the following order:

  1. app.locals
  2. app.settings["view options"] (this is what is added)
  3. renderOptions (from app.render(name, renderOptions, callback) { ... })

The latter ones override the former. I'm not sure if the added test suffice or they are in the right place.

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

Successfully merging this pull request may close these issues.

2 participants