feat(compiler): Introduce InterpolationConfig into component by lacolaco · Pull Request #9158 · angular/angular

lacolaco · 2016-06-11T16:27:31Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/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)

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

Feature

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

Interpolation symbols are defined as const INTERPOLATION_REGEXP.
This is not friendly to collaboration with other templating engines. (e.g. Python jinja)

What is the new behavior?

ComponentMetadata has a new property, interpolation, which has start/end symbols for interpolation.

@Component({
  template: `
  {% someProp %}
  `,
  interpolation: ['{%', '%}']
})
class SomeComp {
  someProp;
}

Does this PR introduce a breaking change?

No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:
This is re-implementation from #8906

cc/ @vicb

lacolaco · 2016-06-11T16:32:31Z

I'm not sure whether that config works well on the offline compiler because I don't know how to test it...

lacolaco · 2016-06-11T16:37:19Z

apply to I18nHtmlParser

vicb · 2016-06-13T02:05:32Z

modules/@angular/compiler/test/metadata_resolver_spec.ts

interpolation: ['{{', '}}'] ?

Oops, here should be new InterpolationConfig('{{', '}}') .
I think array is not type-safe. TypeScript cannot restrict that array's length.

Static checking with Class/Interface vs Runtime checking with compMeta.interpolation.length == 2
Which makes more sense?

We can not instantiate objects in decorators, otherwise they are not statically analyzable. The current version works fine because your object has the same interface as InterpolationConfig (TS uses structural type checking) but IMO is too verbose.

You can use an Array and check the length in the Component ctor

I see. I'll do that. 👍

vicb · 2016-06-13T15:36:29Z

Great work on this PR !

I think you miss html_lexer, please add a test and update the code. Thaanks.

lacolaco · 2016-06-14T12:02:51Z

@vicb Thank you for reviewing!
I saw html_lexer and updated it, but I didn't know how to test whether {% a %} is captured as interpolation.

I wrote following tests at html_lexer_spec.ts

      it('should parse interpolation', () => {
        expect(tokenizeAndHumanizeParts('{{ a }}')).toEqual([
          [HtmlTokenType.TEXT, '{{ a }}'], [HtmlTokenType.EOF]
        ]);

        expect(tokenizeAndHumanizeParts('{% a %}', null, {startSymbol: '{%', endSymbol: '%}'}))
            .toEqual([[HtmlTokenType.TEXT, '{% a %}'], [HtmlTokenType.EOF]]);
      });

Either case, the result is HtmlTokenType.TEXT. I'm not sure whether it is meaningful tests.

In template_parser_spec, I could know that well via BoundTextAst

           expect(humanizeTplAst(parser.parse(component, '{%a%}', [], [], 'TestComp'))).toEqual([
             // unparser doesn't know interpolation config. check only AST type.
             [BoundTextAst, '{{ a }}']
           ]);

Can I check html_lexer as well as template_parser?

vicb · 2016-06-16T01:21:24Z

Sorry for the delay, I'll try to check your PR by EOW. Could you please rebase ? Thanks

vicb · 2016-06-16T01:29:45Z

modules/@angular/compiler/src/assertions.ts

return type :void

vicb · 2016-06-16T01:50:24Z

I have added some comments.

Could you please add some integration test.

It would be have to have nested and sibling component with a different config (also length should be different, ie not 2 and start vs end).

Thanks !

vicb · 2016-06-18T19:30:36Z

modules/@angular/compiler/src/assertions.ts

Use a list BLACKLIST_REGEXPS

vicb · 2016-06-18T19:53:30Z

A few comments but your work is very good and we can probably merge early next week. You can squash the current commits.

Thanks, a few people have tried to implement this but haven't completed - including myself :)

lacolaco · 2016-06-19T00:55:22Z

@vicb Thanks a lot, Victor. I really appreciate your kind help!
The rest of the work is to upgrade ts-api-guardian version after it's released.
I'll squash my commits after that.

lacolaco · 2016-06-19T01:21:45Z

modules/@angular/compiler/src/assertions.ts

Can I use for...of statement? (thinking about ts2dart)

IBR.forEach(pattern => ...)

lacolaco · 2016-06-19T08:01:47Z

I thought we need to be careful about ts2dart transformation for const , RegExp or etc.
Isn't it necessary now?

vicb · 2016-06-19T17:25:19Z

Isn't it necessary now?

If the tests pass you're fine

vicb · 2016-06-20T01:37:15Z

Is the remaining issue linked to api guardian not being updated ?

Please squash your commits.

Thanks again for your great work.

lacolaco · 2016-06-20T01:46:06Z

@vicb I think so. Thanks.

lacolaco · 2016-06-20T01:46:52Z

I've squashed commits in 892f44f

closes angular#9158

… config) (#9367) closes #9158

angular-automatic-lock-bot · 2019-09-08T18:22:37Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Jun 11, 2016

lacolaco force-pushed the laco-component-interpolation-config branch 3 times, most recently from 33a7c54 to ba2b1bc Compare June 12, 2016 05:52

vicb reviewed Jun 13, 2016
View reviewed changes

lacolaco force-pushed the laco-component-interpolation-config branch 2 times, most recently from 5a2052f to 5930d00 Compare June 14, 2016 12:00

vicb reviewed Jun 16, 2016
View reviewed changes

modules/@angular/compiler/src/assertions.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 16, 2016

Choose a reason for hiding this comment

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

return type :void

lacolaco force-pushed the laco-component-interpolation-config branch from 5930d00 to a36ec80 Compare June 16, 2016 04:25

vicb mentioned this pull request Jun 18, 2016

Html Parser improvements #9340

Closed

vicb reviewed Jun 18, 2016
View reviewed changes

modules/@angular/compiler/src/assertions.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jun 18, 2016

Choose a reason for hiding this comment

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

Use a list BLACKLIST_REGEXPS

lacolaco reviewed Jun 19, 2016
View reviewed changes

feat(compiler): Introduce InterpolationConfig into component

892f44f

lacolaco force-pushed the laco-component-interpolation-config branch from 3565e8a to 892f44f Compare June 20, 2016 01:45

vicb pushed a commit to vicb/angular that referenced this pull request Jun 20, 2016

feat(compiler): Introduce InterpolationConfig into component

22f179e

closes angular#9158

vicb mentioned this pull request Jun 20, 2016

feat(compiler): Introduce InterpolationConfig into component #9367

Merged

vicb closed this in #9367 Jun 20, 2016

vicb added a commit that referenced this pull request Jun 20, 2016

feat(compiler): make interpolation symbols configurable (@Component…

1b28cf7

… config) (#9367) closes #9158

lacolaco deleted the laco-component-interpolation-config branch June 21, 2016 00:16

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

Conversation

lacolaco commented Jun 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lacolaco commented Jun 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lacolaco commented Jun 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lacolaco Jun 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb commented Jun 13, 2016

Uh oh!

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

Uh oh!

Uh oh!

vicb commented Jun 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb commented Jun 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb commented Jun 18, 2016

Uh oh!

lacolaco commented Jun 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lacolaco commented Jun 19, 2016

Uh oh!

vicb commented Jun 19, 2016

Uh oh!

vicb commented Jun 20, 2016

Uh oh!

lacolaco commented Jun 20, 2016

Uh oh!

lacolaco commented Jun 20, 2016

Uh oh!

angular-automatic-lock-bot bot commented Sep 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lacolaco commented Jun 11, 2016 •

edited

Loading

lacolaco commented Jun 11, 2016 •

edited

Loading

lacolaco commented Jun 11, 2016 •

edited

Loading

lacolaco Jun 13, 2016 •

edited

Loading

lacolaco commented Jun 14, 2016 •

edited

Loading