feat(template-compiler): experimental template expressions #3376

divmain · 2023-03-02T08:34:29Z

Details

Note: Because this PR adds a lot of tests, I've crafted commits so that you can easily filter the diff in Files changed view to functional changes only (or tests, if that's what interests you).

This PR introduces complex template expressions.

<div attr={value}></div>
<div>{value[currentIdx]}</div>
<div attr={foo ? bar}></div>
<div>{foo?.bar}</div>

Whereas lone identifiers are transformed into corresponding field-access member expressions, parameter variables are not be transformed in this way. For example, the following transformations do not happen:

{(foo) => foo && bar} -> {(foo) => $cmp.foo && $cmp.bar}
{(foo) => foo && bar} -> {($cmp.foo) => foo && $cmp.bar}
{(foo) => foo && bar} -> {($cmp.foo) => $cmp.foo && $cmp.bar}

Instead, the scopes are respected: {(foo) => foo && $cmp.bar}. Similar checks occur for local identifiers introduced via for:each or similar.

This feature is implemented behind a flag, which strongly influenced some some implementation choices.

Most of the code is self-contained in new TS modules.
// TODO [#3370] comments are used liberally throughout, wherever code needs to be stripped (if template expressions are not adopted) or where changes need to be made (if template expressions are adopted).
There should be zero changes to internal IR or external outputs when the flag is disabled. The only snapshot that needed to be updated was related to the newly-exposed flag itself.

Some additional notes:

~~I'm currently tracking the work to be done in pr.todo. This file will be deleted prior to merge.~~
~~There are no new tests. These will be added in a follow-up PR with the experimentalComplexExpressions flag enabled.~~ Added a number of tests for coverage purposes and sanity. Will be adding more tests as a follow-up.

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

✅ No, it does not introduce an observable change.

GUS work item

W-12177613

packages/@lwc/template-compiler/src/codegen/expression.ts

divmain · 2023-03-02T09:39:00Z

packages/@lwc/template-compiler/src/parser/expression-complex/html.ts

+
+        // Start parsing immediately after the opening curly brace.
+        const estreeNode = parseExpressionAt(html, javascriptExprStart, {
+            ecmaVersion: 2022,


ES2022 will be parsed, but none of the newly introduced syntax is permitted. This will just make for better error messages; the component author will be notified about their use of disallowed syntax (if they try to use it) rather than seeing an arcane parsing error.

Are we going to need to periodically update this? Can we just use "latest"? (Which, presumably, will bump when we bump our acorn dep.)

Good question. With the current implementation, the AST is validated using a blocklist for disallowed node types. If we used "latest" here, we'd probably want to invert that to avoid permitting new JS syntax proposals. However, the validation logic would be much more complex if using an allowlist vs blocklist.

Added TMPL_EXPR_ECMASCRIPT_EDITION constant to ensure consistency

divmain · 2023-03-02T09:49:45Z

packages/@lwc/template-compiler/src/parser/index.ts

+    // TODO [#3370]: If complex template expressions are adopted, `preparsedJsExpressions`
+    // should be checked. However, to avoid significant complications in the internal types,
+    // arising from supporting both implementations simultaneously, we will re-parse the
+    // expression here when `ctx.config.experimentalComplexExpressions` is true.
    if (isExpression(value) && !escapedExpression) {
        attrValue = parseExpression(ctx, value, location);


This isn't ideal. The expression at that location has already been parsed (during tokenization) when the experimentalComplexExpressions flag is enabled. Ideally, we should check the preparsedJsExpressions to find the matching one, and use that for the attrValue. However, this would require changes to internal types (like that of attrValue) that cascade into messy changes in multiple places.

To be clear, these are solely type issues. However, to avoid making messy changes across our internal types, I opted to parse the expression a second time, here. This seemed a decent tradeoff for a pilot feature - and can & should be addressed when the flag goes away. I'm open to going the other way if there are strong opinions in favor.

I assume this is why parseExpressionAt is called twice?

packages/@lwc/template-compiler/typings/parse5-internals.d.ts

pr.todo

packages/@lwc/template-compiler/src/codegen/expression.ts

nolanlawson · 2023-03-02T17:16:20Z

packages/@lwc/template-compiler/src/parser/expression-complex/html.ts

+
+        // Start parsing immediately after the opening curly brace.
+        const estreeNode = parseExpressionAt(html, javascriptExprStart, {
+            ecmaVersion: 2022,


Are we going to need to periodically update this? Can we just use "latest"? (Which, presumably, will bump when we bump our acorn dep.)

packages/@lwc/template-compiler/src/parser/expression-complex/html.ts

packages/@lwc/template-compiler/src/parser/expression-complex/validate.ts

packages/@lwc/template-compiler/src/parser/expression.ts

nolanlawson · 2023-03-02T17:26:13Z

packages/@lwc/template-compiler/src/parser/index.ts

+    // TODO [#3370]: If complex template expressions are adopted, `preparsedJsExpressions`
+    // should be checked. However, to avoid significant complications in the internal types,
+    // arising from supporting both implementations simultaneously, we will re-parse the
+    // expression here when `ctx.config.experimentalComplexExpressions` is true.
    if (isExpression(value) && !escapedExpression) {
        attrValue = parseExpression(ctx, value, location);


I assume this is why parseExpressionAt is called twice?

packages/@lwc/template-compiler/typings/parse5-internals.d.ts

divmain · 2023-03-06T23:59:13Z

packages/@lwc/template-compiler/src/parser/expression.ts

 const POTENTIAL_EXPRESSION_RE = /^.?{.+}.*$/;
 const WHITESPACES_RE = /\s/;

 export function isExpression(source: string): boolean {
-    return !!source.match(VALID_EXPRESSION_RE);
+    return source[0] === '{' && source.slice(-1) === '}';


I don't believe this should be an observable/breaking change. The only difference between these two conditions is whether source contains a newline. However, unquoted attribute values (in HTML) cannot contain a new line - so the two are functionally equivalent with the flag disabled.

With the flag enabled, they are not functionally equivalent - the new condition will permit a newline in the attribute value, which is the desired behavior.

Commenting for visibility, in case there is something I'm not considering.

pmdartus · 2023-03-07T09:32:20Z

packages/@lwc/template-compiler/src/shared/types.ts

@@ -61,6 +62,9 @@ export interface MemberExpression extends BaseNode {

 export type Expression = Identifier | MemberExpression;

+// TODO [#3370]: remove experimental template expression flag
+export type ComplexExpression = AcornNode & { value?: any };


The AcornNode type is quite opaque. As far as I understand Acorn return an ESTree-compliant AST. Would it make more sense to have it typed as an ESTree node?

There is also some overlap between the existing Identifier and Expression template AST node, would it make sense to merge those type definitions?

I experimented with this a bit, and any changes to this type resulted in really messy changes required across the codebase. I'm going to leave it for now. However, I've updated the comment to reflect that ComplexExpression should be replaced with ESTree's Node if/when the flag is removed.

packages/@lwc/template-compiler/src/parser/expression-complex/html.ts

pmdartus · 2023-03-07T10:08:20Z

packages/@lwc/template-compiler/src/parser/expression-complex/validate.ts

+        ParserDiagnostics.INVALID_EXPR_COMMENTS_DISALLOWED
+    );
+    invariant(
+        !STATEMENT_TYPES.has(node.type),


Instead of blocking statements nodes, I think it is more future-proof to allow list expressions. This way we would have control over which expressions we allow in the template as new expression types are added to the language.

I originally created an allowlist for node types, but it was messier to translate the decisions in the design doc this into code. I'll probably revisit, but likely in a follow-up PR.

packages/@lwc/template-compiler/src/parser/expression-complex/validate.ts

packages/@lwc/template-compiler/src/parser/index.ts

packages/@lwc/template-compiler/src/codegen/codegen.ts

packages/@lwc/template-compiler/src/parser/expression-complex/types.ts

packages/@lwc/template-compiler/src/parser/expression-complex/html.ts

divmain · 2023-03-11T07:02:57Z

packages/@lwc/template-compiler/src/shared/ast.ts

-export function isExpression(node: Expression | Literal): node is Expression {
-    return node.type === 'Identifier' || node.type === 'MemberExpression';
+export function isExpression(node: BaseNode | Literal): node is Expression {
+    return node.type !== 'Literal';


In a previous iteration of this PR, isExpression had been changed to accept the template expressions flag as its second argument. This led to a deluge of one-line changes where the flag had to be passed in.

To avoid this, I've updated the implementation here. With the flag disabled, the node will have type Identifer, MemberExpression, or Literal. Thus this new condition should be equivalent. The condition also works for the flag-enabled scenario.

ravijayaramappa

First round of review. Reviewing tests now.

packages/@lwc/template-compiler/src/parser/expression-complex/validate.ts

packages/@lwc/template-compiler/src/parser/expression.ts

packages/@lwc/template-compiler/src/parser/expression-complex/html.ts

packages/@lwc/template-compiler/src/parser/parser.ts

packages/@lwc/template-compiler/src/__tests__/fixtures/expression-complex/await/metadata.json

ravijayaramappa · 2023-03-13T20:51:03Z

packages/@lwc/template-compiler/src/__tests__/fixtures/expression-complex/throw/metadata.json

+    "warnings": [
+        {
+            "code": 1052,
+            "message": "Unexpected compilation error: LWC1052: Error parsing attribute: Unexpected token (3:31)",


This error message isn't really useful. The location (3:31) isn't valid as well.

(3:31) refers to throw - that seems correct to me. As for the error message, I'm unsure how to improve it. throw is not valid in expressions; it is only parsed correctly as a statement. Because we've disallowed statements, I can't test that directly. And because the lexer encountered a reserved keyword in an expression, "Unexpected token" is correct.

I'm unsure of the best way to resolve this. I suppose when "Unexpected tokenis encountered, we could look at the indicated location to see if it is athrow` string, but that feels somewhat hacky.

ravijayaramappa

About tests:

Please organize the tests under @lwc/template-compiler/src/__tests__/fixtures/expression-complex into logical folders. The flat hierarchy makes it hard to find a specific tests. A suggestion: valid/, invalid/, invalid/throws/, valid/arrow/ etc.
Missing test cases
- Using complex expression without experimentalComplexExpressions flag enabled should expect diagnostics
- Resolution of variables that are in local scope: identifier declared in for:item, lwc:data

divmain · 2023-03-15T00:02:14Z

To avoid ballooning this PR further, I'm planning a follow-up PR with additional integration tests with other LWC features like for:each. I believe all other feedback has been addressed.

divmain force-pushed the dbustad/template-expressions branch from 6224ace to 1e27a83 Compare March 2, 2023 08:44

divmain changed the title ~~feat: experimental template expressions~~ feat(template-compiler): experimental template expressions Mar 2, 2023

divmain commented Mar 2, 2023

View reviewed changes

packages/@lwc/template-compiler/src/codegen/expression.ts Outdated Show resolved Hide resolved

divmain commented Mar 2, 2023

View reviewed changes

packages/@lwc/template-compiler/typings/parse5-internals.d.ts Show resolved Hide resolved

divmain commented Mar 2, 2023

View reviewed changes

pr.todo Outdated Show resolved Hide resolved

nolanlawson reviewed Mar 2, 2023

View reviewed changes

divmain mentioned this pull request Mar 3, 2023

chore: add feature flag for template expressions #3385

Merged

divmain force-pushed the dbustad/template-expressions branch 2 times, most recently from ef4f825 to ae5fc8c Compare March 6, 2023 10:45

divmain requested review from nolanlawson, pmdartus, ravijayaramappa and ekashida March 6, 2023 11:06

divmain force-pushed the dbustad/template-expressions branch from 9b9fc3d to 296d82d Compare March 6, 2023 23:54

divmain commented Mar 6, 2023

View reviewed changes

divmain force-pushed the dbustad/template-expressions branch from 296d82d to cde0acc Compare March 7, 2023 08:57

pmdartus reviewed Mar 7, 2023

View reviewed changes

divmain commented Mar 7, 2023

View reviewed changes

packages/@lwc/template-compiler/src/parser/expression-complex/types.ts Outdated Show resolved Hide resolved

divmain commented Mar 7, 2023

View reviewed changes

packages/@lwc/template-compiler/src/parser/expression-complex/html.ts Show resolved Hide resolved

nolanlawson reviewed Mar 7, 2023

View reviewed changes

packages/@lwc/template-compiler/src/parser/expression-complex/html.ts Outdated Show resolved Hide resolved

nolanlawson reviewed Mar 7, 2023

View reviewed changes

packages/@lwc/template-compiler/src/parser/expression-complex/html.ts Show resolved Hide resolved

divmain force-pushed the dbustad/template-expressions branch 6 times, most recently from 7b66bcb to d77c47a Compare March 11, 2023 06:29

divmain commented Mar 11, 2023

View reviewed changes

divmain force-pushed the dbustad/template-expressions branch from d77c47a to 95b0a46 Compare March 11, 2023 07:11

divmain requested review from pmdartus and nolanlawson March 11, 2023 07:11

divmain force-pushed the dbustad/template-expressions branch from 95b0a46 to c6d68b2 Compare March 11, 2023 07:25

divmain mentioned this pull request Mar 11, 2023

chore: test downstreams (no merge) #3399

Closed

divmain force-pushed the dbustad/template-expressions branch 2 times, most recently from b014017 to 6076891 Compare March 11, 2023 10:13

ravijayaramappa reviewed Mar 13, 2023

View reviewed changes

packages/@lwc/template-compiler/src/__tests__/fixtures/expression-complex/await/metadata.json Outdated Show resolved Hide resolved

ravijayaramappa reviewed Mar 13, 2023

View reviewed changes

divmain added 3 commits March 14, 2023 16:07

feat(template-compiler): template expressions

59ee1e0

test(template-compiler): template expressions

36778ca

test(integration-karma): template expressions

a04de93

divmain force-pushed the dbustad/template-expressions branch from 6076891 to a04de93 Compare March 14, 2023 23:18

ravijayaramappa approved these changes Mar 15, 2023

View reviewed changes

divmain merged commit 56a191a into master Mar 15, 2023

divmain deleted the dbustad/template-expressions branch March 15, 2023 18:25

divmain mentioned this pull request Feb 9, 2024

feat: new compiler targeting SSR #3880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(template-compiler): experimental template expressions #3376

feat(template-compiler): experimental template expressions #3376

divmain commented Mar 2, 2023 •

edited

Loading

divmain Mar 2, 2023 •

edited

Loading

nolanlawson Mar 2, 2023

divmain Mar 2, 2023 •

edited

Loading

divmain Mar 6, 2023

divmain Mar 2, 2023 •

edited

Loading

nolanlawson Mar 2, 2023

divmain Mar 2, 2023

nolanlawson Mar 2, 2023

nolanlawson Mar 2, 2023

divmain Mar 6, 2023 •

edited

Loading

pmdartus Mar 7, 2023

divmain Mar 11, 2023

pmdartus Mar 7, 2023

divmain Mar 7, 2023

divmain Mar 11, 2023

ravijayaramappa left a comment

ravijayaramappa Mar 13, 2023

divmain Mar 14, 2023 •

edited

Loading

ravijayaramappa left a comment

divmain commented Mar 15, 2023

feat(template-compiler): experimental template expressions #3376

feat(template-compiler): experimental template expressions #3376

Conversation

divmain commented Mar 2, 2023 • edited Loading

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

divmain Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divmain Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divmain Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divmain Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravijayaramappa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divmain Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

ravijayaramappa left a comment

Choose a reason for hiding this comment

divmain commented Mar 15, 2023

divmain commented Mar 2, 2023 •

edited

Loading

divmain Mar 2, 2023 •

edited

Loading

divmain Mar 2, 2023 •

edited

Loading

divmain Mar 2, 2023 •

edited

Loading

divmain Mar 6, 2023 •

edited

Loading

divmain Mar 14, 2023 •

edited

Loading