fix($compile): correctly denormalize templates that do not use standard interpolation symbols #14610

petebacondarwin · 2016-05-13T20:49:51Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

FIX

What is the current behavior? (You can also link to an open issue here)

See #6493

What is the new behavior (if this is a feature change)?

Fixed

Does this PR introduce a breaking change?

No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

** STILL NEED TO ADD DOCS **

In order to support third party modules that do not use the same interpolation
symbols as the main app, we implemented denormalization (see dfe9983).

This required that 3rd party modules always used the standad {{ and }}
interpolation symbols, so that we could correctly denormalise them to the
current app's symbols.

The problem with this became apparent when an app template using new symbols
inadvertently contained one of the standard interpolation symbols.

E.g. <div data-context='{"context":{"id":3,"type":"page"}}">

The double closing curly braces were being incorrectly denormalised.

This commit fixes this by allowing the compiler to be configured to know
what symbols are being used in templates from a given module.

Now modules can specify that what interpolation symbols they use and the
compiler will denormalize appropriately.

Closes #6493
Closes #6453

…rd interpolation symbols In order to support third party modules that do not use the same interpolation symbols as the main app, we implemented denormalization (see dfe9983). This required that 3rd party modules always used the standad `{{` and `}}` interpolation symbols, so that we could correctly denormalise them to the current app's symbols. The problem with this became apparent when an app template using new symbols inadvertently contained one of the standard interpolation symbols. E.g. `<div data-context='{"context":{"id":3,"type":"page"}}">` The double closing curly braces were being incorrectly denormalised. This commit fixes this by allowing the compiler to be configured to know what symbols are being used in templates from a given module. Now modules can specify that what interpolation symbols they use and the compiler will denormalize appropriately. Closes angular#6493 Closes angular#6453

lgalfaso · 2016-05-14T10:44:49Z

test/ng/compileSpec.js

+        });
+
+      module('symbol-test', function($interpolateProvider, $compileProvider) {
+        $interpolateProvider.startSymbol('##');


these two lines should not be there, as with them, the test makes no sense.

lgalfaso · 2016-05-14T10:45:54Z

I see this as a feature. @petebacondarwin would it be possible to know why you think it is a fix?

petebacondarwin · 2016-05-14T12:31:48Z

To clarify this a bit more. If I change my interpolation symbols (in my app) to [[ and ]] then I would expect that a component with this template

<div>{a:{b:42}}</div>

to output

<div>{a:{b:42}}</div>

But it doesn’t. See http://plnkr.co/edit/s9KVOZ9srU0JfkQYPrCf?p=preview

lgalfaso · 2016-05-14T12:36:04Z

Ok, but then I think that should be true in the test even without the
configuration changes to $interpolateProvider

On Saturday, May 14, 2016, Pete Bacon Darwin notifications@github.com
wrote:

To clarify this a bit more. If I change my interpolation symbols (in my
app) to [[ and ]] then I would expect that a component with this template

{a:{b:42}}

should output

{a:{b:42}}

But it doesn’t. See http://plnkr.co/edit/s9KVOZ9srU0JfkQYPrCf?p=preview

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#14610 (comment)

petebacondarwin · 2016-05-14T12:55:23Z

Without the changes to the interpolate provider then no denormalisation occurs so the bug doesn't occur in the same way. If we have not changed the symbols when we accept that we should escape }} ourselves if they appear in the template as content.

lgalfaso · 2016-05-14T12:58:09Z

Without the changes to the interpolate provider then no denormalisation occurs so the bug doesn't occur in the same way. If we have not changed the symbols when we accept that we should escape }} ourselves if they appear in the template as content.

Agree, but then $compileProvider.moduleSymbols('symbol-test', '##', ']]'); does not mean "The interpolation symbols for all templates in this module are ## and ]]" that is what most people would expect.

petebacondarwin · 2016-05-14T13:06:49Z

Ahah! @lgalfaso so what you are saying is that we have another issue:
if the symbols in a template do not match those in the application, then we need to escape any instance of the application symbols, in the templates, too, before we denormalise. Right?

lgalfaso · 2016-05-14T13:24:56Z

implementation details a side, yes, the issue is the one stated.

petebacondarwin · 2016-05-14T18:52:28Z

@lgalfaso here are better tests:

    it('should denormalise interpolation symbols in templates correctly, if different to the current symbols', function() {
      angular.module('symbol-test', [])
        .directive('myDirective', function() {
          return {
            template: '<span foo=\'{"ctx":{"id":3}}\'>[[1 + 2]]</span>'
          };
        });

      module('symbol-test', function($compileProvider) {
        $compileProvider.moduleSymbols('symbol-test', '[[', ']]');
      });

      inject(function($compile) {
        element = $compile('<div><div my-directive></div></div>')($rootScope);
        expect(element.children('div').children('span').attr('foo')).toBe('{"ctx":{"id":3}}');
        expect(element.text()).toEqual('{{1 + 2}}');
        $rootScope.$apply();
        expect(element.text()).toEqual('3');
      });
    });


    it('should escape the current interpolation symbols in templates, before denormalising, if the template module symbols are different', function() {
      angular.module('symbol-test', [])
        .directive('myDirective', function() {
          return {
            template: '<span>{[1 + 2]}[[3 + 4]]</span>'
          };
        });

      module('symbol-test', function($interpolateProvider, $compileProvider) {
        $interpolateProvider.startSymbol('{[');
        $interpolateProvider.endSymbol(']}');
        $compileProvider.moduleSymbols('symbol-test', '[[', ']]');
      });

      inject(function($compile) {
        element = $compile('<div><div my-directive></div></div>')($rootScope);
        expect(element.text()).toEqual('\\{\\[1 + 2\\]\\}{[3 + 4]}');
        $rootScope.$apply();
        expect(element.text()).toEqual('3');
      });
    });

The second one fails at the moment.

…rd interpolation symbols In order to support third party modules that do not use the same interpolation symbols as the main app, we implemented denormalization (see dfe9983). This required that 3rd party modules always used the standad `{{` and `}}` interpolation symbols, so that we could correctly denormalise them to the current app's symbols. The problem with this became apparent when an app template using new symbols inadvertently contained one of the standard interpolation symbols. E.g. `<div data-context='{"context":{"id":3,"type":"page"}}">` The double closing curly braces were being incorrectly denormalised. This commit fixes this by allowing the compiler to be configured to know what symbols are being used in templates from a given module. Now modules can specify that what interpolation symbols they use and the compiler will denormalize appropriately. Closes angular#6493 Closes angular#6453

petebacondarwin · 2016-05-14T19:11:07Z

I added these tests and updated the code to make them pass.

lgalfaso · 2016-05-15T08:18:30Z

@petebacondarwin I think that the solution will have to be somehow more complex. Eg

it('this works', function() {
  angular.module('symbol-test', [])
    .directive('myDirective', function() {
      return {
        template: '<span foo=\'{"ctx":{"id":3}}\'>Hi</span>'
      };
    });

  module('symbol-test');
  inject(function($compile) {
    element = $compile('<div><div my-directive></div></div>')($rootScope);
    $rootScope.$apply();
    expect(element.find('span').attr('foo')).toEqual('{"ctx":{"id":3}}');
  });
});

it('so this should work too', function() {
  angular.module('symbol-test', [])
    .directive('myDirective', function() {
      return {
        template: '<span foo=\'{"ctx":{"id":3}}\'>Hi</span>'
      };
    })
    .config(function($compileProvider) {
      $compileProvider.moduleSymbols('symbol-test', '[[', ']]');
    });

  module('symbol-test');
  inject(function($compile) {
    element = $compile('<div><div my-directive></div></div>')($rootScope);
    $rootScope.$apply();
    expect(element.find('span').attr('foo')).toEqual('{"ctx":{"id":3}}');
  });
});

gkalpak · 2016-05-16T10:41:21Z

I just realized there is another issue with template denormalization: We are not taking [ng-non-bindable] into account - we still replace the interpolation symbols 😞
(Or is this a feature ?)

gkalpak · 2016-05-16T10:46:43Z

test/ng/compileSpec.js

+        expect(element.children('div').children('span').attr('foo')).toBe('{"ctx":{"id":3\\}\\}');
+        expect(element.text()).toEqual('{{1 + 2}}');
+        $rootScope.$apply();
+        expect(element.text()).toEqual('3');


What about .attr('foo') ? I believe it is still escaped and I think this is not what we want...

That is what @lgalfaso was referring to in his test: #14610 (comment)

:-(

petebacondarwin · 2016-05-16T11:05:23Z

Hmm looks like this could be more painful than it is worth...

lgalfaso · 2016-05-16T11:13:44Z

Hmm looks like this could be more painful than it is worth...

yep

petebacondarwin · 2016-05-16T12:14:03Z

Perhaps another one for the "known issues" pile?

gkalpak · 2016-05-16T12:16:58Z

another one

More like "a couple more" 😛

Closes #14610 Closes #6493

googlebot added the cla: yes label May 13, 2016

petebacondarwin added type: bug component: $compile frequency: low needs: review needs: public api severity: broken expected use branch: 1.5.x labels May 13, 2016

petebacondarwin added this to the 1.5.x milestone May 13, 2016

lgalfaso reviewed May 14, 2016
View reviewed changes

gkalpak reviewed May 16, 2016
View reviewed changes

petebacondarwin added a commit that referenced this pull request May 23, 2016

docs($interpolate): add known issue about custom interpolation symbols

5930b8e

Closes #14610 Closes #6493

petebacondarwin closed this in c8e09c8 May 23, 2016

fix($compile): correctly denormalize templates that do not use standard interpolation symbols #14610

fix($compile): correctly denormalize templates that do not use standard interpolation symbols #14610

Uh oh!

Conversation

petebacondarwin commented May 13, 2016

Uh oh!

lgalfaso May 14, 2016

Choose a reason for hiding this comment

Uh oh!

lgalfaso commented May 14, 2016

Uh oh!

petebacondarwin commented May 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgalfaso commented May 14, 2016

Uh oh!

petebacondarwin commented May 14, 2016

Uh oh!

lgalfaso commented May 14, 2016

Uh oh!

petebacondarwin commented May 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgalfaso commented May 14, 2016

Uh oh!

petebacondarwin commented May 14, 2016

Uh oh!

petebacondarwin commented May 14, 2016

Uh oh!

lgalfaso commented May 15, 2016

Uh oh!

gkalpak commented May 16, 2016

Uh oh!

gkalpak May 16, 2016

Choose a reason for hiding this comment

Uh oh!

petebacondarwin May 16, 2016

Choose a reason for hiding this comment

Uh oh!

petebacondarwin commented May 16, 2016

Uh oh!

lgalfaso commented May 16, 2016

Uh oh!

petebacondarwin commented May 16, 2016

Uh oh!

gkalpak commented May 16, 2016

Uh oh!

Uh oh!

petebacondarwin commented May 14, 2016 •

edited

Loading

petebacondarwin commented May 14, 2016 •

edited

Loading