feat(template-compiler): binding ast parser #1498

ekashida · 2019-09-09T01:50:31Z

Details

Template parser that outputs an AST for component data dependencies.

Does this PR introduce breaking changes?

✅ No, it does not introduce breaking changes.

diervo · 2019-09-10T04:53:39Z

@ekashida couple notes:

let's create a test to make sure:

<template if:true={x}>
  <x-foo></x-foo>
</template>

Outputs the same as

  <x-foo if:true{x}></x-foo>

Also let's remove all nodes that are not components

apapko

This is shaping up really nicely Eugene. I do propose 3 changes to consider:

slim down the attribute object shape by eliminating 'object' / 'property' properties in lieu of collapsing them into a single type/value object. See details inline
decouple binding parser into its own function for cleaner api that can be easily moved into its own package if needed
debate to move the binding parser functionality into lwc-platform for initial internal consumption

apapko · 2019-09-10T16:45:27Z

packages/@lwc/template-compiler/src/__tests__/fixtures.spec.ts

+                    prettier.format(expectedCode, { parser: 'babel' })
+                );
+            }
+            if (category === 'parser') {


should this be else if?

I guess there's no difference since category can't be both 'compiler' and 'parser'

packages/@lwc/template-compiler/src/__tests__/fixtures/parser/for-each-template/expected.json

packages/@lwc/template-compiler/src/__tests__/fixtures/parser/if-inline/expected.json

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

apapko · 2019-09-10T17:51:20Z

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

 export { IRElement } from './shared/types';
 export { Config } from './config';

-export function parse(source: string, config?: Config): TemplateParseResult {
+export function parse(source: string, config?: Config): BindingParseResult | TemplateParseResult {


I would prefer exposing binding parser as a separate function to keep things more decoupled and not intermingle return types.

Now that i'm thinking about it, would it make more sense to move bindingTransform API into lwc-platform to keep it internal, at least until we hash out all the details? I can see the value of providing data dependencies to external devs, just unsure if now is the time. Thoughts @caridy @diervo @ekashida ?

I think it's fine since we have it behind a flag called experimentalDataBindingAST

packages/@lwc/template-compiler/src/transform/binding.ts

apapko · 2019-09-10T17:58:51Z

packages/@lwc/template-compiler/src/transform/binding.ts

+                type: 'BindingASTAttribute',
+                name: attr.name,
+                // Force BabelTemplateExpression because IRTemplateExpression.value
+                // can also be a literal...is that a bug?


is this specific to literal attribute value?
prop="literal value"

Yeah, you can't have a literal value in an expression so not sure about it

ekashida · 2019-09-11T23:36:52Z

packages/@lwc/template-compiler/src/__tests__/parser-binding.spec.ts

+                    <foo-bar for:each={items} for:item="item" microwave={oven}></foo-bar>
+                </template>
+            `);
+            expect(inlineRoot).toEqual(templateRoot);


apapko · 2019-09-12T01:01:22Z

Looks good, but i would like to revisit the binding attribute object shape.

apapko · 2019-09-12T15:51:41Z

Just realized that the commend i made was hidden under resolved conversations. Re-posting here:

I'm still not sure about the shape of the BindingASTAttribute object, specifically that it has an 'expression' and an 'object' and properties, i feel like it makes the shape inflexible. What would the shape look like if you had a deeply nested expression? eg:

data={item.data.first.second.third.level}

I think folding the type and value into a single object is more intuitive and easy to parse, for example:

"attributes": [
    {
        "name": "data",
        "value": {
                value: "item.data.first.second.third",
                type: "expression"
        }
    }
}

or

attributes = [{
        "data": {
            value: "item.data.first.second.third",
            type: "expression"
        },
  }]

ekashida · 2019-09-12T18:58:37Z

I think folding the type and value into a single object is more intuitive and easy to parse

@apapko What do you mean by "intuitive"? ASTs are generally not meant for humans to look at. Parsing an expression such as item.data.first.second.third into a recursive object/property structure will make it very easy for programs to analyze the AST.

diervo · 2019-09-19T02:30:20Z

packages/@lwc/template-compiler/src/transform/binding.ts

+interface BindingASTComponentNode {
+    attributes: BindingASTAttribute[];
+    children: Array<BindingASTNode | BindingASTSlotNode | BindingASTComponentNode>;
+    component: true;


component: true is probably is not needed

diervo

Changes:

Add stringLiteral attributes
Change the type for directives and remove the directive member (ex. ["if"])
remove component:true

diervo

Changes:

Add stringLiteral attributes
Change the type for directives and remove the directive member (ex. ["if"])
remove component:true

diervo

Changes:

Add stringLiteral attributes
Change the type for directives and remove the directive member (ex. ["if"])
remove component:true

diervo

Changes:

Add stringLiteral attributes
Change the type for directives and remove the directive member (ex. ["if"])
remove component:true

diervo

Changes:

Add stringLiteral attributes
Change the type for directives and remove the directive member (ex. ["if"])
remove component:true

diervo

Changes:

Add stringLiteral attributes
Change the type for directives and remove the directive member (ex. ["if"])
remove component:true

jbenallen · 2019-09-19T14:11:57Z

packages/@lwc/template-compiler/src/__tests__/fixtures/parser/deeply-nested/expected.json

+                                    "children": [
+                                        {
+                                            "type": "BindingASTComponentNode",
+                                            "tag": "foo-header",


This object is missing the slot information, specifically the slot name to which it is assigned. That will be relevant as the owner of that named slot could wrap that named slot with its own conditional.

jbenallen · 2019-09-19T14:30:39Z

packages/@lwc/template-compiler/src/__tests__/fixtures/parser/for-each-template/actual.html

+    <div class="foo-list-container" tabindex={tabIndex}>
+        <foo-list>
+            <template for:each={items} for:item="item" for:index="index">
+                <foo-list-item data={item.data} key={item.id}></foo-list-item>


Can you add a test for passing index to one of foo-list-item's props?

jbenallen · 2019-09-19T14:59:15Z

packages/@lwc/template-compiler/src/transform/binding.ts

+        const object = expression.object as babelTypes.MemberExpression;
+        return {
+            type: 'BindingASTMemberExpression',
+            object: pruneMemberExpression(object),


I assume this covers patterns like {grandfather.father.son}. Can you include a test that exercises this?

https://github.com/salesforce/lwc/pull/1498/files#diff-30821f5231fb50ec080df13ee0b89782

apapko · 2019-09-19T16:52:45Z

packages/@lwc/template-compiler/src/transform/binding.ts

@@ -35,7 +35,7 @@ interface BindingASTAttribute {
 }

 interface BindingASTNode {
-    children: Array<BindingASTNode | BindingASTComponentNode>;
+    children: Array<BindingASTNode | BindingASTSlotNode | BindingASTComponentNode>;


i like the introduction of types!

one thought: would it make sense to have the slot and the component nodes be a subclass of an BindingASTNode? This way we can use BindingASTNode type as a super type for all interfaces/functions and potentially reuse any common functionality if needed. For ex:

interface BindingASTSlotNode { children: Array<BindingASTNode>; }

I believe this was addressed in the refactor

apapko · 2019-09-19T17:00:02Z

packages/@lwc/template-compiler/src/transform/binding.ts

+            children: [],
+        };
+    }
+    throw new Error(`Expected element ${element.tag} to be a slot.`);


should we say Expected a <slot> element, instead received ${element.tag}

Updated to Expected <slot> element but received <${element.tag}> element.

apapko · 2019-09-19T17:06:26Z

packages/@lwc/template-compiler/src/transform/binding.ts

-                        // node between the parent and component node.
-                        const directiveNode = transformToASTNode(currentElement);
+                        // node between the parent and child node.
+                        const directiveNode = transformToDirectiveNode(currentElement);
                        parentASTNode.children.push(directiveNode);


I see that we have tests for slots with directives. Would it be worth to add a test for a custom component with a directive as well?

See for-each-inline

(cherry picked from commit 1d77e5e)

* feat(template-compiler): binding ast parser (#1498) (cherry picked from commit 1d77e5e) * chore: locally define isUndefined

(cherry picked from commit 1d77e5e)

This reverts commit 1d77e5e.

ekashida requested review from diervo, pmdartus, apapko and jbenallen September 9, 2019 01:50

ekashida added 7 commits September 9, 2019 20:58

chore: define parser option to output data binding AST

209a06a

feat: initial commit of binding ast transform

1a09898

chore: wire up option to transform

bb23a35

fix: support inline forEach/forOf

a40ff4a

fix: support if:true/if:false

765af0e

feat: test fixture setup for parser

2c1ef6b

test: fixtures for parser

1b1f67a

ekashida force-pushed the ekashida/binding-ast branch from ca5010d to ba279ee Compare September 10, 2019 03:59

ekashida added 2 commits September 9, 2019 21:01

fix: add missing license header

4403425

chore: license header check to ignore fixtures-parser files

4350da8

ekashida force-pushed the ekashida/binding-ast branch from ba279ee to 4350da8 Compare September 10, 2019 04:01

chore: move compiler fixtures into fixtures/compiler

4ac7ff4

ekashida added 2 commits September 9, 2019 22:03

test: clean support for both compiler and parser fixtures

331fb9b

test: fix templates

ed62d6f

apapko reviewed Sep 10, 2019

View reviewed changes

refactor: include only the relevant nodes in the ast

87bb528

ekashida force-pushed the ekashida/binding-ast branch from 6301f19 to 87bb528 Compare September 11, 2019 15:00

ekashida added 3 commits September 11, 2019 14:50

chore: add back if-directive

a6f5f60

chore: add back ast hierchy

dc6dea2

test: unit tests

5810ff1

ekashida commented Sep 11, 2019

View reviewed changes

chore: optimization tweaks

1bdb705

test: complex template

1aa4193

diervo reviewed Sep 19, 2019

View reviewed changes

diervo approved these changes Sep 19, 2019

View reviewed changes

diervo suggested changes Sep 19, 2019

View reviewed changes

jbenallen suggested changes Sep 19, 2019

View reviewed changes

chore: include static attribute values and tweak types

c559919

apapko reviewed Sep 19, 2019

View reviewed changes

ekashida added 2 commits September 20, 2019 06:25

chore: for:index

001df31

chore: review feedback

a42cd01

ekashida force-pushed the ekashida/binding-ast branch from 5ac2ae7 to a42cd01 Compare September 19, 2019 21:56

chore: include test for:index

8ade991

diervo approved these changes Sep 23, 2019

View reviewed changes

ekashida added 4 commits September 24, 2019 03:46

refactor: move parser fixtures into own directory

e93f216

revert: move compiler fixtures back

c19bad9

fix: ignore fixtures-parser/ when checking license headers

6fce210

test: add fixture for boolean and string attributes

707d511

ekashida merged commit 1d77e5e into master Sep 23, 2019

ekashida deleted the ekashida/binding-ast branch September 23, 2019 20:20

ekashida added a commit that referenced this pull request Sep 24, 2019

feat(template-compiler): binding ast parser (#1498)

ead3ae8

(cherry picked from commit 1d77e5e)

ekashida added a commit that referenced this pull request Sep 24, 2019

feat(template-compiler): binding ast parser (#1498)

bbfc9ec

(cherry picked from commit 1d77e5e)

ekashida added a commit that referenced this pull request Sep 24, 2019

feat(template-compiler): binding ast parser (#1532)

1a281e2

* feat(template-compiler): binding ast parser (#1498) (cherry picked from commit 1d77e5e) * chore: locally define isUndefined

ekashida added a commit that referenced this pull request Sep 24, 2019

feat(template-compiler): binding ast parser (#1498)

9084fd0

(cherry picked from commit 1d77e5e)

ekashida added a commit that referenced this pull request Sep 24, 2019

feat(template-compiler): binding ast parser (#1498)

8e3ea04

(cherry picked from commit 1d77e5e)

ekashida added a commit that referenced this pull request Sep 25, 2019

feat(template-compiler): binding ast parser (#1498)

3efb630

ekashida added a commit that referenced this pull request Sep 27, 2019

Revert "feat(template-compiler): binding ast parser (#1498)"

80015f1

This reverts commit 1d77e5e.

ekashida added a commit that referenced this pull request Sep 27, 2019

Revert "feat(template-compiler): binding ast parser (#1498)" (#1538)

3e87c2c

This reverts commit 1d77e5e.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(template-compiler): binding ast parser #1498

feat(template-compiler): binding ast parser #1498

ekashida commented Sep 9, 2019

diervo commented Sep 10, 2019

apapko left a comment •

edited

apapko Sep 10, 2019

ekashida Sep 11, 2019

apapko Sep 10, 2019

apapko Sep 10, 2019

ekashida Sep 11, 2019

apapko Sep 10, 2019

ekashida Sep 11, 2019

ekashida Sep 11, 2019

apapko commented Sep 12, 2019 •

edited

apapko commented Sep 12, 2019

ekashida commented Sep 12, 2019 •

edited

diervo Sep 19, 2019 •

edited

diervo left a comment

diervo left a comment

diervo left a comment

diervo left a comment

diervo left a comment

diervo left a comment

jbenallen Sep 19, 2019

jbenallen Sep 19, 2019

ekashida Sep 19, 2019

jbenallen Sep 19, 2019

ekashida Sep 19, 2019

apapko Sep 19, 2019

apapko Sep 19, 2019

ekashida Sep 19, 2019

apapko Sep 19, 2019

ekashida Sep 19, 2019

apapko Sep 19, 2019

ekashida Sep 19, 2019

feat(template-compiler): binding ast parser #1498

feat(template-compiler): binding ast parser #1498

Conversation

ekashida commented Sep 9, 2019

Details

Does this PR introduce breaking changes?

diervo commented Sep 10, 2019

apapko left a comment • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapko commented Sep 12, 2019 • edited

apapko commented Sep 12, 2019

ekashida commented Sep 12, 2019 • edited

diervo Sep 19, 2019 • edited

Choose a reason for hiding this comment

diervo left a comment

Choose a reason for hiding this comment

diervo left a comment

Choose a reason for hiding this comment

diervo left a comment

Choose a reason for hiding this comment

diervo left a comment

Choose a reason for hiding this comment

diervo left a comment

Choose a reason for hiding this comment

diervo left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapko left a comment •

edited

apapko commented Sep 12, 2019 •

edited

ekashida commented Sep 12, 2019 •

edited

diervo Sep 19, 2019 •

edited