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

feat: allow a custom escapeExpression for an env #1523

Open
wants to merge 1 commit into
base: 4.x
Choose a base branch
from

Conversation

thekiwi
Copy link

@thekiwi thekiwi commented Apr 17, 2019

We use handlebars.js for both XML templating and JSON templating within the same application. As such, we'd like to be able to override the escapeExpressions function on a single Handlebars environment, rather than globally. This is a small PR that allows us to do that, by first attempting to get the environment's version of escapeExpression and falling back to the Utils version if that is falsey.

Usage (or how we might end up using it):

const hb = Handlebars.create();
hb.escapeExpression = str => JSON.stringify(str).slice(1, -1);
// ... continue using 'hb' as normal

Please let me know if anything is unclear or if you'd like me to make additional changes.

  • Please don't start pull requests for security issues. Instead, file a report at https://www.npmjs.com/advisories/report?package=handlebars
  • Maintain the existing code style
  • Are focused on a single change (i.e. avoid large refactoring or style adjustments in untouched code if not the primary goal of the pull request)
  • Have good commit messages
  • Have tests
  • Have the typings (lib/handlebars.d.ts) updated on every API change. If you need help, updating those, please mention that in the PR description.
  • Don't significantly decrease the current code coverage (see coverage/lcov-report/index.html)
  • Currently, the 4.x-branch contains the latest version. Please target that branch in the PR.

@thekiwi
Copy link
Author

thekiwi commented Apr 25, 2019

Hey @nknapp, just wondering whether you had a chance to look at this? Is it a small enough addition that you would be happy to merge and release? Thanks!

@nknapp
Copy link
Collaborator

nknapp commented Apr 26, 2019

Sorry, I haven't had a thorough look yet. But I think something similar is part of our plans avoid security vulnerabilities in the future. I would like to defer until then to avoid unnecessary api changes.

@nknapp nknapp added the feature label Sep 20, 2019
@nknapp
Copy link
Collaborator

nknapp commented Sep 29, 2019

I would like to merge this, but the API is not what I would want to have. I have to do a general API discusssion with @wycats for overriding this like that (helperMissing, blockHelperMissing, escapeExpression). I don't know when I will be able to do that and it might fall in the "we need a spec" category.

I would think of an interface similar to

Handlebars.registerHook('escapeExpression', (string) => ...);
// or 
Handlebars.hooks.escapeExpression((string) => ...)

Technically, this is not a language feature, so I'm not sure ich 'we need a spec' applies here, but in the end, that @wycats decision.

@nknapp nknapp added this to the Backlog milestone Oct 27, 2019
@thekiwi
Copy link
Author

thekiwi commented Oct 30, 2019

@nknapp I'd be very happy with your proposed change. I made my PR change as small as possible to minimise the impact, but totally understand if you'd like to implement this as a proper supported and documented API.

@nknapp
Copy link
Collaborator

nknapp commented Oct 30, 2019

In #1559, I added the hooks property to the runtime. I'm still not confident about a good name for the function(s) to register a hook

  • Handlebars.on('escapeExpression', (expression) => ...)) would match the common code from Node.js when it comes to events, but this is not an event.
  • Handlebars.registerHook('escapeExpression', (expression) => ...)) would match the current helper-registration in a way. But it bugs me that there is a string defining the hook, because there is no arbitrary set of hooks to be replaced.
  • Handlebars.hooks.escapeExpression((expression) => ...)) is not possible at the moment, because the hooks object already contains hooks, but actually, the hooks should be private, so maybe we should call them _hooks anyway. Still, the method name does not suggest that the escapeExpression-function is being replaced.
  • Handlebars.replace.escapeExpression((expression) => ...)) would be more descriptive. But it feels weird having a replace property. And it would sound wrong for helperMissing.
  • Handlebars.hookInto.escapeExpression() ...

I don't know. Does anyone have a good idea, or knows how other frameworks solve this?

In any case, it might be helpful to pass the old method into the hook function as well, so that the wrapper can be decide to call it. Or maybe not in the first stage.

@nknapp
Copy link
Collaborator

nknapp commented Nov 17, 2019

@thekiwi would it be OK for you to have a runtime option to override "escapeExpression"?

@thekiwi
Copy link
Author

thekiwi commented Nov 18, 2019

@nknapp It should be, are you able to provide a snippet demonstrating how I would use it?

@nknapp
Copy link
Collaborator

nknapp commented Nov 19, 2019

It would be something like

const template = Handlebars.compile('{{name}}');
const output = template({ name: 'Jonny "the hound" Walker' }, {
  hooks: {
     escapeExpression: string => string.replace(/(["'\\])/g,'\\$1')
  }
}

Copy link

@KrishnaPG KrishnaPG left a comment

Choose a reason for hiding this comment

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

This is better than having to override the global utils.escapeExpression.

Till some other mechanism (such as hooks() etc.) is made available, this should be strongly considered for merging.

@silverwind
Copy link

Ideally I too would like to see a interface that does not require the use of Handlebars.create, but I tend to agree this is mergeable as-is, just to unblock a number of people who would like to use Handlebars for non-HTML templating with proper escaping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature item for language spec Used for issues that describe things that should be covered by a spec topic for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants