feat(compiler): support tagged template literals in code generator #39122

bjarkler · 2020-10-05T22:36:16Z

Add a TaggedTemplateExpr to represent tagged template literals in
Angular's syntax tree (more specifically Expression in output_ast.ts).
Also update classes that implement ExpressionVisitor to add support for
tagged template literals in different contexts, such as JIT compilation
and conversion to JS.

Partial support for tagged template literals had already been
implemented to support the $localize tag used by Angular's i18n
framework. Where applicable, this code was refactored to support
arbitrary tags, although completely replacing the i18n-specific support
for the $localize tag with the new generic support for tagged template
literals may not be completely trivial, and is left as future work.

PR Checklist

Please check if your PR fulfills the following requirements:

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

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

bjarkler · 2020-10-06T13:32:01Z

presumit

petebacondarwin

This is looking nice. Do we care to support tagged template literals in View Engine, given that it is deprecated?

packages/compiler-cli/src/ngtsc/translator/src/translator.ts

packages/compiler-cli/src/transformers/node_emitter.ts

bjarkler · 2020-10-06T14:49:14Z

Thanks for the comments @petebacondarwin!

Do we care to support tagged template literals in View Engine, given that it is deprecated?

Since I'm a bit new to the Angular codebase, do you mind pointing out which components in my PR are ViewEngine-specific? And for full ViewEngine support, do you think there's anything missing?

For background I'm working on adding support for Trusted Types in Angular. Tagged template literals make it possible to define an API that only accepts a constant string as input (i.e. with a regular function it's impossible to distinguish between fun('constant') and fun(variable), but tagged template literals allow you to distinguish between fun'constant' and fun'${variable}' (with ` instead of ')), which is something that we may require. And since Trusted Types are a security feature we are considering supporting ViewEngine as well.

petebacondarwin · 2020-10-06T14:59:13Z

@bjarkler - the main part of this PR that is ViewEngine specific, I believe, is the node_emitter.ts file.
So if you do want to support ViewEngine you are all sorted, I think.

packages/compiler/src/output/output_ast.ts

petebacondarwin · 2020-10-07T20:41:08Z

In this Trusted Types requirement, will the template literal string be something that the application developer puts into a template or will the compiler generate them somehow? Can you point me at some background to this? The reason I ask is that the "raw" strings are dependent upon the source of the literal.

bjarkler · 2020-10-07T20:58:14Z

For the Trusted Types use case, consider the following Angular template:

<div>
  <iframe id="frame" srcdoc="<h1>Hello</h1>"></iframe>
</div>

When the template is compiled, any attributes with constant values are hoisted by Angular into a separate "consts" array in the compiled source:

consts: [["id", "frame", "srcdoc", "<h1>Hello</h1>"]]

If an application enforces Trusted Types, this will later cause a Trusted Types violation, as setting the srcdoc of an iframe requires a TrustedHTML object. So the idea is to introduce a function/tag called ɵɵtrustHtml which converts a constant string into a TrustedHTML, and alter the compiler to output the following consts array instead:

consts: [["id", "frame", "srcdoc", ɵɵtrustHtml`<h1>Hello</h1>`]]

Using ɵɵtrustHtml improperly (either within Angular or by application developers) can undermine the security that Trusted Types introduce. We would like to use a tagged template literal as opposed to a function call to minimize risk of doing that by accident.

Supporting raw strings or even sourceSpans are not strict requirements for this I believe, but since I was adding support for this I figured it would be better to try to do it properly, in case there were other use cases in the future.

bjarkler · 2020-10-07T21:00:33Z

(But FWIW, in my current implementation I use the sourceSpan of the corresponding constant in the Angular template.)

petebacondarwin

This looks much better. Nice work!

petebacondarwin · 2020-10-08T18:27:39Z

Just need to get @alxhub or @JoostK to review it too.
I assume you can kick off the presubmit yourself @bjarkler?

bjarkler · 2020-10-08T18:28:13Z

Just need to get @alxhub or @JoostK to review it too.
I assume you can kick off the presubmit yourself @bjarkler?

Will do! Thanks again.

bjarkler · 2020-10-08T19:37:08Z

second presubmit

bjarkler · 2020-10-09T10:51:33Z

CC @engelsdamien @koto

bjarkler · 2020-10-16T15:26:05Z

@alxhub, can you take a look?

IgorMinar

This looks good to me, but I'd like @alxhub to take a look as well. Thank you!

IgorMinar · 2020-11-19T23:16:55Z

packages/compiler/src/output/abstract_js_emitter.ts

+ * array.
+ */
+const makeTemplateObjectPolyfill =
+    '(this&&this.__makeTemplateObject||function(e,t){return Object.defineProperty?Object.defineProperty(e,"raw",{value:t}):e.raw=t,e})';


it's strange that we need to provide this polyfill ourselves.

Shouldn't it come from tslib?

Could we check if the current file has import for this method and if not add an import from tslib? Is that feasible @petebacondarwin @alxhub ?

If this PR were aimed at Ivy only, I think it would definitely be possible. The way to do this would be to treat it as an ExternalExpr that refers to {moduleName: 'tslib', name: 'makeTemplateObject'} (or whatever the tslib export is).

Then, we'll have to ensure that in JIT mode (which is where AbstractJsEmitterVisitor ends up being used in Ivy), that external reference will have be made resolvable by the R3JitReflector to the real tslib function.

View Engine might be able to handle emitting such an import, but I've not ever tried that so I don't know for sure.

Since this PR was originally sent we've now decided not to support Trusted Types in ViewEngine, and I believe the $localize tag was already Ivy-only. So I don't think there's anything preventing us from dropping the ViewEngine-specific parts from this PR and following @alxhub's proposal. I'd be happy to see how far I can take this but might need some guidance with the R3JitReflector part.

ok, how about this - let's remove the VE code paths and land this PR as is. @bjarkler can the follow up with @alxhub on fixing the polyfill issue in a separate PR.

Sounds good @IgorMinar. I've now removed the VE-specific code.

alxhub · 2020-11-19T23:58:02Z

packages/compiler/src/output/abstract_js_emitter.ts

+ * array.
+ */
+const makeTemplateObjectPolyfill =
+    '(this&&this.__makeTemplateObject||function(e,t){return Object.defineProperty?Object.defineProperty(e,"raw",{value:t}):e.raw=t,e})';


If this PR were aimed at Ivy only, I think it would definitely be possible. The way to do this would be to treat it as an ExternalExpr that refers to {moduleName: 'tslib', name: 'makeTemplateObject'} (or whatever the tslib export is).

Then, we'll have to ensure that in JIT mode (which is where AbstractJsEmitterVisitor ends up being used in Ivy), that external reference will have be made resolvable by the R3JitReflector to the real tslib function.

View Engine might be able to handle emitting such an import, but I've not ever tried that so I don't know for sure.

Add a TaggedTemplateExpr to represent tagged template literals in Angular's syntax tree (more specifically Expression in output_ast.ts). Also update classes that implement ExpressionVisitor to add support for tagged template literals in different contexts, such as JIT compilation and conversion to JS. Partial support for tagged template literals had already been implemented to support the $localize tag used by Angular's i18n framework. Where applicable, this code was refactored to support arbitrary tags, although completely replacing the i18n-specific support for the $localize tag with the new generic support for tagged template literals may not be completely trivial, and is left as future work.

…39122) Add a TaggedTemplateExpr to represent tagged template literals in Angular's syntax tree (more specifically Expression in output_ast.ts). Also update classes that implement ExpressionVisitor to add support for tagged template literals in different contexts, such as JIT compilation and conversion to JS. Partial support for tagged template literals had already been implemented to support the $localize tag used by Angular's i18n framework. Where applicable, this code was refactored to support arbitrary tags, although completely replacing the i18n-specific support for the $localize tag with the new generic support for tagged template literals may not be completely trivial, and is left as future work. PR Close #39122

…rator (angular#39122)" This reverts commit 5631daa.

…rator (#39122)" (#40050) This reverts commit 5631daa. PR Close #40050

angular-automatic-lock-bot · 2021-01-08T16:55:09Z

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 Oct 5, 2020

pullapprove bot requested a review from alxhub October 5, 2020 22:36

bjarkler force-pushed the tagged-templates branch from 8dbfc88 to bf70ea9 Compare October 6, 2020 12:49

petebacondarwin requested changes Oct 6, 2020

View reviewed changes

packages/compiler-cli/src/ngtsc/translator/src/translator.ts Outdated Show resolved Hide resolved

packages/compiler-cli/src/transformers/node_emitter.ts Outdated Show resolved Hide resolved

bjarkler force-pushed the tagged-templates branch from bf70ea9 to 1a3b684 Compare October 6, 2020 14:30

bjarkler requested a review from petebacondarwin October 7, 2020 14:23

petebacondarwin added refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release labels Oct 7, 2020

petebacondarwin reviewed Oct 7, 2020

View reviewed changes

packages/compiler/src/output/output_ast.ts Outdated Show resolved Hide resolved

petebacondarwin self-requested a review October 7, 2020 20:26

bjarkler force-pushed the tagged-templates branch from 1a3b684 to 21184cd Compare October 8, 2020 17:08

petebacondarwin approved these changes Oct 8, 2020

View reviewed changes

atscott added the area: compiler Issues related to `ngc`, Angular's template compiler label Oct 13, 2020

ngbot bot added this to the needsTriage milestone Oct 13, 2020

IgorMinar self-requested a review October 21, 2020 17:30

IgorMinar approved these changes Nov 19, 2020

View reviewed changes

alxhub approved these changes Nov 20, 2020

View reviewed changes

bjarkler force-pushed the tagged-templates branch 2 times, most recently from cd11097 to a5c457f Compare November 30, 2020 16:26

bjarkler force-pushed the tagged-templates branch from a5c457f to 5a49982 Compare November 30, 2020 18:24

bjarkler mentioned this pull request Dec 1, 2020

WIP(core): reduce scope of Trusted Types policies #39910

Closed

14 tasks

IgorMinar added the action: merge The PR is ready for merge by the caretaker label Dec 5, 2020

ngbot bot modified the milestones: needsTriage, Backlog Dec 5, 2020

mhevery closed this in ef89274 Dec 8, 2020

josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 9, 2020

Revert "feat(compiler): support tagged template literals in code gene…

2927436

…rator (angular#39122)" This reverts commit 5631daa.

josephperrott added a commit that referenced this pull request Dec 9, 2020

Revert "feat(compiler): support tagged template literals in code gene…

73ab673

…rator (#39122)" (#40050) This reverts commit 5631daa. PR Close #40050

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(compiler): support tagged template literals in code generator #39122

feat(compiler): support tagged template literals in code generator #39122

bjarkler commented Oct 5, 2020

bjarkler commented Oct 6, 2020

petebacondarwin left a comment

bjarkler commented Oct 6, 2020

petebacondarwin commented Oct 6, 2020

petebacondarwin commented Oct 7, 2020

bjarkler commented Oct 7, 2020

bjarkler commented Oct 7, 2020

petebacondarwin left a comment

petebacondarwin commented Oct 8, 2020

bjarkler commented Oct 8, 2020

bjarkler commented Oct 8, 2020

bjarkler commented Oct 9, 2020

bjarkler commented Oct 16, 2020

IgorMinar left a comment

IgorMinar Nov 19, 2020

alxhub Nov 19, 2020

bjarkler Nov 20, 2020

IgorMinar Nov 23, 2020

bjarkler Nov 30, 2020

alxhub Nov 19, 2020

angular-automatic-lock-bot bot commented Jan 8, 2021

feat(compiler): support tagged template literals in code generator #39122

feat(compiler): support tagged template literals in code generator #39122

Conversation

bjarkler commented Oct 5, 2020

PR Checklist

PR Type

bjarkler commented Oct 6, 2020

petebacondarwin left a comment

Choose a reason for hiding this comment

bjarkler commented Oct 6, 2020

petebacondarwin commented Oct 6, 2020

petebacondarwin commented Oct 7, 2020

bjarkler commented Oct 7, 2020

bjarkler commented Oct 7, 2020

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin commented Oct 8, 2020

bjarkler commented Oct 8, 2020

bjarkler commented Oct 8, 2020

bjarkler commented Oct 9, 2020

bjarkler commented Oct 16, 2020

IgorMinar left a comment

Choose a reason for hiding this comment

IgorMinar Nov 19, 2020

Choose a reason for hiding this comment

alxhub Nov 19, 2020

Choose a reason for hiding this comment

bjarkler Nov 20, 2020

Choose a reason for hiding this comment

IgorMinar Nov 23, 2020

Choose a reason for hiding this comment

bjarkler Nov 30, 2020

Choose a reason for hiding this comment

alxhub Nov 19, 2020

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Jan 8, 2021