feat: lwc:if, lwc:elseif, and lwc:else directives #2985

jye-sf · 2022-08-04T19:34:17Z

Details

WIP

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

⚠️ Yes, it does include an observable change.

GUS work item

jye-sf · 2022-08-04T19:37:05Z

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

@@ -355,18 +310,30 @@ function parseChildren(
    parse5ParentLocation: parse5.ElementLocation
 ): void {
    const children = (parse5Utils.getTemplateContent(parse5Parent) ?? parse5Parent).childNodes;
+
+    let siblingContext: IfBlock | undefined;


This is the loop where we iterate through the siblings of a particular node and where can keep track of any context we need from sibling to sibling.

jye-sf · 2022-08-04T19:38:33Z

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

@@ -45,6 +46,12 @@ interface ParentWrapper {
    current: ParentNode;
 }

+interface Scope {


Enhanced the parser scope to keep track of the context we need from any previous sibling scopes + any context we need to preserve about our current scope. This is currently only used for keeping track of lwc:if, lwc:elseif, and lwc:else relationships but can be enhanced in the future if we need to keep track of more info from siblings.

…pe directly

jye-sf · 2022-08-04T20:28:24Z

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

@@ -355,6 +310,8 @@ function parseChildren(
    parse5ParentLocation: parse5.ElementLocation
 ): void {
    const children = (parse5Utils.getTemplateContent(parse5Parent) ?? parse5Parent).childNodes;
+
+    ctx.beginSiblingScope();


I refactored the sibling tracking into the context object so we shouldn't need to have manual logic here anymore.

jye-sf · 2022-08-04T20:30:07Z

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

@@ -364,12 +321,23 @@ function parseChildren(
            } else if (parse5Utils.isTextNode(child)) {
                const textNodes = parseText(ctx, child);
                parent.children.push(...textNodes);
+                // Non whitespace text nodes interrupt any context we may be carrying
+                if (textNodes.length > 0) {
+                    ctx.clearSiblingScope();


IMO for this block and one below with comments, we do something like ctx.visitTextNode or ctx.visitCommentNode and let the logic for when to clear the sibling scope live inside the context object. Any thoughts on naming?

jye-sf · 2022-08-04T20:34:53Z

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

        parseIfBlock,
        parseElseifBlock,
        parseElseBlock,
+        parseForEach,
+        parseForOf,
+        parseIf,
    ];
    for (const parser of parsers) {


For this loop, I refactored the logic that adds the node to the current scope and attaches them to the appropriate parent node to the actual parsing functions for each directive. The reasoning is twofold:

half the directives now have "special" logic on what to do when attaching them, so it's no longer very streamlined to do it here.

there's no longer a 1:1 mapping between Node type and parser function here, so distinguishing what to do with with a given node at this level is trickier. 2 of the 6 nodes here would need some combination of ast.isXXXXBlock and checking for function equality to determine what to do with them. We already have all that info readily available in the parsing functions.

jye-sf · 2022-08-04T20:36:32Z

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

@@ -66,6 +69,7 @@ export default class ParserCtx {
     * Each scope corresponds to the original parse5.Element node.
     */
    private readonly scopes: ParentNode[][] = [];


Some renaming is necessary here.

@jmsjtu I had some trouble determining what kind of scope these are supposed to be to distinguish them from siblingScope. Would it be elementScope? nodeScope?

This specifically stores the ForBlock, If, IfBlock, ElseBlock, Element, Component and Slot.

The type that encompasses all of these is ParentNode.

The intention of this array is to keep track of all the AST nodes that are generated from a single HTML Element.

I think elementScope would be appropriate in this case to help distinguish it from siblingScope.

jmsjtu · 2022-08-05T19:30:31Z

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

            }
        });
    }
+    ctx.beginSiblingScope();


Should this be ctx.clearSiblingScope()?

Yes, that's a memory leak waiting to happen.

jmsjtu · 2022-08-05T19:42:31Z

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

 ): IfBlock | undefined {
    const ifBlockAttribute = parsedAttr.pick('lwc:if');
    if (!ifBlockAttribute) {
        return;
    }

-    const hasElseBlock = ctx.findSibling(ast.isIfBlock);
+    const hasElseBlock = ctx.findSibling(ast.isElseBlock);


I think we should rename findSibling in the ctx since there's a new sibling array, maybe we can call it something like findInCurrentScope.

* feat: lwc:if, lwc:elseif, and lwc:else directives (#2985) * feat: refactor parser context to support new behavior for slot name tracking (#2990) * feat(template-compiler): add codegen portion for else-if directives (#2995) * fix: use Fragment VNodes for if-elseif-else children (#3047) Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com> Co-authored-by: James Tu <j.tu@salesforce.com>

jye-sf added 3 commits August 3, 2022 11:17

feat: additional tests

8ecacb7

feat: cache preceding node context when parsing siblings

0df9997

feat: compatibility with for:each + tests

e7744f9

jye-sf commented Aug 4, 2022

View reviewed changes

jye-sf added 2 commits August 4, 2022 13:04

feat: remove unneeded context property and store the prev sibling sco…

1331b0c

…pe directly

feat: refactor need to manually track sibling context

52ec20b

jye-sf commented Aug 4, 2022

View reviewed changes

feat: throw appropriate errors when used with if:true and if:false

e939781

jmsjtu reviewed Aug 5, 2022

View reviewed changes

jye-sf added 3 commits August 5, 2022 14:18

feat: introduce ElseifBlock and refactor

feb46c9

fix: address PR comments

41cdc2e

fix: update next error code

e5f97ba

jmsjtu approved these changes Aug 5, 2022

View reviewed changes

jye-sf merged commit fe34953 into salesforce:if-else-mob Aug 5, 2022

jye-sf deleted the jye/if-else-parser branch August 5, 2022 22:31

nolanlawson mentioned this pull request Aug 8, 2022

Enable else on the template after using if:true | false #2503

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: lwc:if, lwc:elseif, and lwc:else directives #2985

feat: lwc:if, lwc:elseif, and lwc:else directives #2985

jye-sf commented Aug 4, 2022

jye-sf Aug 4, 2022

jye-sf Aug 4, 2022 •

edited

Loading

jye-sf Aug 4, 2022

jye-sf Aug 4, 2022

jye-sf Aug 4, 2022

jye-sf Aug 4, 2022

jmsjtu Aug 5, 2022

jmsjtu Aug 5, 2022 •

edited

Loading

jye-sf Aug 5, 2022

jmsjtu Aug 5, 2022

feat: lwc:if, lwc:elseif, and lwc:else directives #2985

feat: lwc:if, lwc:elseif, and lwc:else directives #2985

Conversation

jye-sf commented Aug 4, 2022

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

Choose a reason for hiding this comment

jye-sf Aug 4, 2022 • 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

jmsjtu Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jye-sf Aug 4, 2022 •

edited

Loading

jmsjtu Aug 5, 2022 •

edited

Loading