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

sub, sup, bold and small helpers undocumented? #1628

Closed
AndrewLeedham opened this issue Dec 11, 2019 · 14 comments
Closed

sub, sup, bold and small helpers undocumented? #1628

AndrewLeedham opened this issue Dec 11, 2019 · 14 comments

Comments

@AndrewLeedham
Copy link
Contributor

I recently came across some weird behaviour where the template {{#url}}{{small}}{{/url}} when passed the data { url: 'https://google.com' } wraps the url in small html tags, initially I thought this is just built-in helpers, but looking at the docs it is not documented as one. Furthermore, when logging the built-in helpers and partials small is not referenced anywhere, I also found the same to be true for sub, sup and bold. Searching the codebase for these terms also yields nothing. I am left very confused. Where is this behaviour coming from? and is it possible to turn it off?

I wrote a little JSFiddle to demonstrate the issue: https://jsfiddle.net/d0ve69xL/

@nknapp
Copy link
Collaborator

nknapp commented Dec 11, 2019

I think, this works because bold is a function on String.prototype.

console.log("test".bold()) // "<b>bold</b>"

@AndrewLeedham
Copy link
Contributor Author

Ah yes, looks like that is it.
image

This seems like an unintended side effect?

@AndrewLeedham
Copy link
Contributor Author

@nknapp happy to put in a PR if you can point me in the right direction?

@nknapp
Copy link
Collaborator

nknapp commented Dec 11, 2019

Unintended? Probably. But it also works and I don't know who relies on it working.

We can change it in 5.0, but not in 4.x
In 4.x, we could make a switch and turn the default value in 5.0 this is something I wanted to do for security reasons anyway.

I would make it a runtime option (boolean flag)
It needs to be changed in the nameLookup function of the compiler and in the lookup helper.

The difficulty is that old templates need to work with the current runtime and vice versa.

@nknapp
Copy link
Collaborator

nknapp commented Dec 11, 2019

I tried this in #1594 but decided to create a simpler solution first.

@nknapp
Copy link
Collaborator

nknapp commented Dec 11, 2019

I'd like to know why the behaviour is a problem for you.

@AndrewLeedham
Copy link
Contributor Author

I am trying to progressively convert a project from mustache to handlebars. One of the partials that is used a lot is the button which wraps the template in a url if (mustache style). So I end up with a template loosely like this:

{{#url}}
<a href="{{url}}" class="{{#small}}small{{/small}}">
  text
</a>
{{/url}}

Since small is a function by default, if just url is passed the small property is always Truthy, whereas it would be undefined in mustache.

@nknapp
Copy link
Collaborator

nknapp commented Dec 11, 2019

And mustache does not resolve prototype properties?

@nknapp
Copy link
Collaborator

nknapp commented Dec 11, 2019

I'm just wondering: One of the major features of Handlebars was its mustache-compatibility. I just wonder why this has never occured before and why this case isn't caught by the mustache-testcases that handlebars runs against in each build.

@AndrewLeedham
Copy link
Contributor Author

Mustache does not resolve the prototype properties no:

@nknapp
Copy link
Collaborator

nknapp commented Dec 11, 2019

I think it is not that easy to implement this check correctly. It should be a runtime option, but because compiler and runtime must remain compatible in 4.x, we cannot just add a function to the "container" object in the runtime that performs the check. If the compiled template calls the function and it is not there because the runtime is older than the compiler, then people will complain (I made that mistake already).

In 5.0, that would be OK, but still, the "lookup"-helper does not have access to the "container" and cannot check the runtime option. And the "lookup"-helper should behave equally to a direct lookup.

I have plans for releasing a 5.0 version. Apart from code-cleanup, this this a feature I wanted anyway, for security reasons. It would be great if you start it.

@AndrewLeedham
Copy link
Contributor Author

What is the roadmap for 5.0 is there likely to be an alpha/beta version anytime soon? Is it feature complete, would it be plausible to use in a development environment?

@nknapp
Copy link
Collaborator

nknapp commented Dec 12, 2019

My plan for 5.0 is to

  • publish the changes that are currently on the master branch Move main development back to master-branch #1566,
  • bump webpack and babel to the current versions (which is not trivial and may cause breaking changes)
  • disallow access to prototype properties

My main motivation is to go back to the master branch and eliminate the other two pain points.
I cannot give you an exact timeline, though, because I work on the project only when I have time to do so. But that is also true for the 4.x branch.

@nknapp
Copy link
Collaborator

nknapp commented Jan 11, 2020

Disallowing prototype access is released in 4.6. This issue should be fixed now.

@nknapp nknapp closed this as completed Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants