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

Broken when used with custom interpolation symbols #264

Open
slavafomin opened this issue Jan 24, 2017 · 5 comments
Open

Broken when used with custom interpolation symbols #264

slavafomin opened this issue Jan 24, 2017 · 5 comments

Comments

@slavafomin
Copy link

slavafomin commented Jan 24, 2017

Hello!

Thank you for this great library!

However, when it's used in projects with custom interpolation symbols it's not working (Angular is throwing expression syntax exceptions).

For example, we use the following interpolation symbols to avoid conflicts with Twig templating engine which we use server-side:

  • {~ instead of {{
  • ~} instead of }}

It's not safe to rely on default interpolation symbols when developing a library, because they can be easily overridden in end-developer's project. It's better to rely on ngBind directive.

This Plunk demonstrates the issue:
https://plnkr.co/edit/NXnu2P5FLzcjYn2OjKNe?p=preview

Thanks!

@slavafomin
Copy link
Author

slavafomin commented Jan 24, 2017

As I see here: https://github.com/wix/angular-tree-control/blob/master/angular-tree-control.js#L294-L305.

You are using Angular's $compile service to pre-compile the template according to the specified options. This is the source of this bug.

Here's the very simple "naive" solution that works:

if(!template) {
    var open = $interpolate.startSymbol();
    var close = $interpolate.endSymbol();
    template =
        '<ul ' + open + 'options.ulClass' + close + ' >' +
        '<li ng-repeat="node in node.' + open + 'options.nodeChildren' + close + ' | filter:filterExpression:filterComparator ' + open + 'options.orderBy' + close + '" ng-class="headClass(node)" ' + open + 'options.liClass' + close + '' +
        'set-node-to-data>' +
        '<i class="tree-branch-head" ng-class="iBranchClass()" ng-click="selectNodeHead(node)"></i>' +
        '<i class="tree-leaf-head ' + open + 'options.iLeafClass' + close + '"></i>' +
        '<div class="tree-label ' + open + 'options.labelClass' + close + '" ng-class="[selectedClass(), unselectableClass()]" ng-click="selectNodeLabel(node)" tree-transclude></div>' +
        '<treeitem ng-if="nodeExpanded()"></treeitem>' +
        '</li>' +
        '</ul>';
}

However, is it really necessary to use $compile for such a trivial purpose? I think the better approach would be to just concatenate template source with options using vanilla JavaScript. Actually this will improve performance of this operation, because template compilation is a complex and slow process.

slavafomin pushed a commit to slavafomin/angular-tree-control that referenced this issue Jan 24, 2017
@slavafomin
Copy link
Author

slavafomin commented Jan 24, 2017

I've created a PR which fixes this issue. It contains the code from my example above and takes into consideration effective interpolation symbols. I believe this is the best approach to preserve backward-compatibility for developers who use custom templates (i.e. templateUrl).

@slavafomin
Copy link
Author

I've updated the PR with cleaner solution:

if(!template) {
    template =
        '<ul {{options.ulClass}} >' +
        '<li ng-repeat="node in node.{{options.nodeChildren}} | filter:filterExpression:filterComparator {{options.orderBy}}" ng-class="headClass(node)" {{options.liClass}}' +
        'set-node-to-data>' +
        '<i class="tree-branch-head" ng-class="iBranchClass()" ng-click="selectNodeHead(node)"></i>' +
        '<i class="tree-leaf-head {{options.iLeafClass}}"></i>' +
        '<div class="tree-label {{options.labelClass}}" ng-class="[selectedClass(), unselectableClass()]" ng-click="selectNodeLabel(node)" tree-transclude></div>' +
        '<treeitem ng-if="nodeExpanded()"></treeitem>' +
        '</li>' +
        '</ul>';
    template = template
        .replace(/{{/g, $interpolate.startSymbol())
        .replace(/}}/g, $interpolate.endSymbol())
    ;
}

The following addition to the original code should do the trick:

template = template
    .replace(/{{/g, $interpolate.startSymbol())
    .replace(/}}/g, $interpolate.endSymbol())
;

@lie-nielsen
Copy link

I had same problem. This looks good. Why no merge.

@slavafomin
Copy link
Author

We should ask @yoavaa ;)

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