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

Pass data to Handlebars as options #177

Merged
merged 2 commits into from
May 26, 2017
Merged

Pass data to Handlebars as options #177

merged 2 commits into from
May 26, 2017

Conversation

kjarmicki
Copy link
Contributor

FIxes regression introduced in #171 where noEscape was not passed into Handlebars anymore.

@@ -73,7 +73,7 @@ function filterFiles(filters) {
function render(template, data, callback) {
let rendered;
try {
rendered = Handlebars.compile(template)(data);
rendered = Handlebars.compile(template, data)(data);
Copy link
Member

@dlmr dlmr May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding { noEscape: true } directly here, it is something we actually always want to use and means that we do not need to "pollute" data by overloading it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want { noEscape: true } for all the usages of render? In renderTemplateFiles it was previously used without it, as metadata was taken directly from metalsmith.
Maybe we should instead open up render to use separate data and options and pass noEscape in options only when rendering log message?

Copy link
Member

@dlmr dlmr May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we always want to use noEscape and it was a bug that this was not done before. We could address this later however since it might be unrelated to the changes suggested here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good time to address it since this PR is related to that, so if you don't mind I'd like to adjust it with that change sometime tomorrow :)

@dlmr dlmr merged commit 426fda3 into rocjs:master May 26, 2017
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.

None yet

2 participants