fix: use MarkoTagBody for control flow (#2358)

marko-js · Nov 11, 2024 · 76951d8 · 76951d8
1 parent 0ec5b96
commit 76951d8
Show file tree

Hide file tree

Showing 20 changed files with 134 additions and 115 deletions.
diff --git a/.changeset/mean-ants-perform.md b/.changeset/mean-ants-perform.md
@@ -0,0 +1,11 @@
+---
+"@marko/translator-default": patch
+"@marko/translator-tags": patch
+"@marko/babel-utils": patch
+"@marko/compiler": patch
+"marko": patch
+"@marko/runtime-tags": patch
+"@marko/translator-interop-class-tags": patch
+---
+
+Always use MarkoTagBody AST nodes for control flow (even with attribute tags). This fixes a regression with the @marko/tags-api-preview and is more accurate to what is actually happening, especially from a variable scoping perspective.
diff --git a/packages/babel-utils/src/tags.js b/packages/babel-utils/src/tags.js
@@ -213,7 +213,10 @@ export function findParentTag(path) {
 }
 
 export function findAttributeTags(path, attributeTags = []) {
-  path.get("attributeTags").forEach((child) => {
+  const attrTags = path.node.body.attributeTags
+    ? path.get("body").get("body")
+    : path.get("attributeTags");
+  attrTags.forEach((child) => {
     if (isAttributeTag(child)) {
       attributeTags.push(child);
     } else if (isTransparentTag(child)) {

diff --git a/packages/compiler/src/babel-plugin/parser.js b/packages/compiler/src/babel-plugin/parser.js
@@ -542,34 +542,50 @@ export function parseMarko(file) {
 
         if (node.body.body.length) {
           const body = [];
+          // When we have a control flow with mixed body and attribute tag content
+          // we move any scriptlets, comments or empty nested control flow.
+          // This is because they initially ambiguous as to whether
+          // they are part of the body or the attributeTags.
+          // Otherwise we only move scriptlets.
           for (const child of node.body.body) {
             if (
               t.isMarkoScriptlet(child) ||
-              (isControlFlow &&
-                (t.isMarkoComment(child) ||
-                  (child.tagDef?.controlFlow && !child.body.body.length)))
+              (isControlFlow && t.isMarkoComment(child))
             ) {
-              // When we have a control flow with mixed body and attribute tag content
-              // we move any scriptlets, comments or empty nested control flow.
-              // This is because they initially ambiguous as to whether
-              // they are part of the body or the attributeTags.
+              attributeTags.push(child);
+            } else if (
+              isControlFlow &&
+              child.tagDef?.controlFlow &&
+              !child.body.body.length
+            ) {
+              child.body.attributeTags = true;
               attributeTags.push(child);
             } else {
               body.push(child);
             }
           }
 
-          if (isControlFlow && body.length) {
-            onNext();
-            throw file.buildCodeFrameError(
-              body[0],
-              "Cannot have attribute tags and body content under a control flow tag.",
-            );
+          if (isControlFlow) {
+            if (body.length) {
+              onNext();
+              throw file.buildCodeFrameError(
+                body[0],
+                "Cannot have attribute tags and body content under a control flow tag.",
+              );
+            }
+
+            node.attributeTags = body;
+            node.body.body = attributeTags;
+            node.body.attributeTags = true;
           } else {
             node.body.body = body;
           }
 
           attributeTags.sort(sortByStart);
+        } else if (isControlFlow) {
+          node.attributeTags = [];
+          node.body.body = attributeTags;
+          node.body.attributeTags = true;
         }
 
         if (isControlFlow) {

diff --git a/packages/compiler/src/babel-types/types/definitions.js b/packages/compiler/src/babel-types/types/definitions.js
@@ -175,6 +175,10 @@ const MarkoDefinitions = {
         ]),
         default: [],
       },
+      attributeTags: {
+        validate: assertValueType("boolean"),
+        default: false,
+      },
     },
   },
 

diff --git a/packages/translator-default/src/tag/attribute-tag.js b/packages/translator-default/src/tag/attribute-tag.js
@@ -27,7 +27,10 @@ export function analyzeAttributeTags(rootTag) {
 
   while (i < visit.length) {
     const tag = visit[i++];
-    for (const child of tag.get("attributeTags")) {
+    const attrTags = tag.node.body.attributeTags
+      ? tag.get("body").get("body")
+      : tag.get("attributeTags");
+    for (const child of attrTags) {
       if (isAttributeTag(child)) {
         assertNoArgs(child);
         const tagDef = getTagDef(child) || {};
@@ -89,17 +92,7 @@ export function analyzeAttributeTags(rootTag) {
         parentTags.push(child);
         visit.push(child);
       } else if (isTransparentTag(child)) {
-        switch (getContentType(child)) {
-          case ContentType.mixed:
-            throw child.buildCodeFrameError(
-              "Cannot mix @tags with other content when under a control flow.",
-            );
-          case ContentType.attribute:
-            visit.push(child);
-            break;
-          case ContentType.render:
-            break;
-        }
+        visit.push(child);
       }
     }
   }
@@ -147,45 +140,6 @@ function getAttrTagObject(tag) {
   return attrs;
 }
 
-function getContentType(tag) {
-  const { node } = tag;
-  const cached = contentTypeCache.get(node);
-  if (cached !== undefined) return cached;
-
-  const body = tag.get("body").get("body");
-  let hasAttributeTag = false;
-  let hasRenderBody = false;
-
-  for (const child of body) {
-    if (isAttributeTag(child)) {
-      hasAttributeTag = true;
-    } else if (isTransparentTag(child)) {
-      switch (getContentType(child)) {
-        case ContentType.mixed:
-          contentTypeCache.set(node, ContentType.mixed);
-          return ContentType.mixed;
-        case ContentType.attribute:
-          hasAttributeTag = true;
-          break;
-        case ContentType.render:
-          hasRenderBody = true;
-          break;
-      }
-    } else if (!child.isMarkoScriptlet() && !child.isMarkoComment()) {
-      hasRenderBody = true;
-    }
-
-    if (hasAttributeTag && hasRenderBody) {
-      contentTypeCache.set(node, ContentType.mixed);
-      return ContentType.mixed;
-    }
-  }
-
-  const result = hasAttributeTag ? ContentType.attribute : ContentType.render;
-  contentTypeCache.set(node, result);
-  return result;
-}
-
 function removeDashes(str) {
   return str.replace(/-([a-z])/g, matchToUpperCase);
 }

diff --git a/packages/translator-default/src/tag/index.js b/packages/translator-default/src/tag/index.js
@@ -50,8 +50,8 @@ export default {
     if (!isAttributeTag(path)) {
       if (
         !tagDef &&
-        path.node.attributeTags.length &&
         path.hub.file.markoOpts.ignoreUnrecognizedTags &&
+        (path.node.attributeTags.length || path.node.body.attributeTags) &&
         !isDynamicTag(path)
       ) {
         moveIgnoredAttrTags(path);
@@ -261,9 +261,13 @@ function isMarkoFile(request) {
 }
 
 function moveIgnoredAttrTags(parentTag) {
-  if (!parentTag.node.attributeTags.length) return;
+  const attrTags = parentTag.node.body.attributeTags
+    ? parentTag.get("body").get("body")
+    : parentTag.get("attributeTags");
 
-  for (const attrTag of parentTag.get("attributeTags")) {
+  if (!attrTags.length) return;
+
+  for (const attrTag of attrTags) {
     if (attrTag.isMarkoTag()) {
       if (isAttributeTag(attrTag)) {
         attrTag.set(

diff --git a/packages/translator-default/src/taglib/core/conditional/translate-else.js b/packages/translator-default/src/taglib/core/conditional/translate-else.js
@@ -15,10 +15,6 @@ export function exit(path) {
       );
   }
 
-  ifStatement.alternate = t.blockStatement(
-    path.node.attributeTags.length
-      ? path.node.attributeTags
-      : path.node.body.body,
-  );
+  ifStatement.alternate = t.blockStatement(path.node.body.body);
   path.remove();
 }
diff --git a/packages/translator-default/src/taglib/core/conditional/util.js b/packages/translator-default/src/taglib/core/conditional/util.js
@@ -12,11 +12,7 @@ export function buildIfStatement(path, args) {
 
   const ifStatement = t.ifStatement(
     args.length === 1 ? args[0] : t.sequenceExpression(args),
-    t.blockStatement(
-      path.node.attributeTags.length
-        ? path.node.attributeTags
-        : path.node.body.body,
-    ),
+    t.blockStatement(path.node.body.body),
   );
 
   let nextPath = path.getNextSibling();

diff --git a/packages/translator-default/src/taglib/core/translate-for.js b/packages/translator-default/src/taglib/core/translate-for.js
@@ -7,7 +7,7 @@ export function exit(path) {
     attributes,
     body: { params },
   } = node;
-  const body = node.attributeTags.length ? node.attributeTags : node.body.body;
+  const body = node.body.body;
   const namePath = path.get("name");
   const ofAttr = findName(attributes, "of");
   const inAttr = findName(attributes, "in");

diff --git a/packages/translator-default/src/taglib/core/translate-while.js b/packages/translator-default/src/taglib/core/translate-while.js
@@ -24,11 +24,7 @@ export function exit(path) {
     withPreviousLocation(
       t.whileStatement(
         getArgOrSequence(path),
-        t.blockStatement(
-          path.node.attributeTags.length
-            ? path.node.attributeTags
-            : path.node.body.body,
-        ),
+        t.blockStatement(path.node.body.body),
       ),
       path.node,
     ),

diff --git a/packages/translator-default/src/util/optimize-vdom-create.js b/packages/translator-default/src/util/optimize-vdom-create.js
@@ -72,6 +72,7 @@ const analyzeStaticVisitor = {
       let isStatic =
         isNativeTag(path) &&
         !path.node.attributeTags.length &&
+        !path.node.body.attributeTags &&
         !path.node.body.params.length &&
         !path.node.arguments &&
         !hasUserKey(path);

diff --git a/packages/translator-tags/src/core/for.ts b/packages/translator-tags/src/core/for.ts
@@ -10,6 +10,7 @@ import { AccessorChar, WalkCode } from "@marko/runtime-tags/common/types";
 
 import { assertNoSpreadAttrs } from "../util/assert";
 import { getKnownAttrValues } from "../util/get-known-attr-values";
+import { getParentTag } from "../util/get-parent-tag";
 import { isStatefulReferences } from "../util/is-stateful";
 import {
   type Binding,
@@ -64,7 +65,7 @@ declare module "@marko/compiler/dist/types" {
 export default {
   analyze(tag) {
     const tagExtra = (tag.node.extra ??= {});
-    const isAttrTag = !!tag.node.attributeTags.length;
+    const isAttrTag = tag.node.body.attributeTags;
     let allowAttrs: string[];
     assertNoVar(tag);
     assertNoArgs(tag);
@@ -105,7 +106,7 @@ export default {
     const section = getOrCreateSection(tag);
 
     if (isOnlyChildInParent(tag)) {
-      const parentTag = tag.parentPath.parent as t.MarkoTag;
+      const parentTag = getParentTag(tag)!.node;
       const parentTagName = (parentTag.name as t.StringLiteral)?.value;
       (parentTag.extra ??= {})[kNativeTagBinding] ??= createBinding(
         "#" + parentTagName,
@@ -127,7 +128,7 @@ export default {
   translate: translateByTarget({
     html: {
       enter(tag) {
-        if (tag.node.attributeTags.length) return;
+        if (tag.node.body.attributeTags) return;
 
         const tagBody = tag.get("body");
         const bodySection = getSectionForBody(tagBody);
@@ -157,16 +158,17 @@ export default {
         }
       },
       exit(tag) {
-        if (tag.node.attributeTags.length) return;
+        if (tag.node.body.attributeTags) return;
 
         const tagBody = tag.get("body");
         const tagSection = getSection(tag);
         const bodySection = getSectionForBody(tagBody)!;
         const { node } = tag;
         const tagExtra = node.extra!;
         const isStateful = isStatefulReferences(tagExtra.referencedBindings);
+        const parentTag = getParentTag(tag);
         const nodeRef = isOnlyChildInParent(tag)
-          ? tag.parentPath.parent.extra![kNativeTagBinding]!
+          ? parentTag!.node.extra![kNativeTagBinding]!
           : tag.node.extra![kForMarkerBinding]!;
         const forAttrs = getKnownAttrValues(node);
         const forType = getForType(node)!;
@@ -177,7 +179,7 @@ export default {
         let keyExpression: t.Expression | undefined;
 
         if (isStateful && isOnlyChildInParent(tag)) {
-          tag.parentPath.parent.extra![kSerializeMarker] = true;
+          parentTag!.node.extra![kSerializeMarker] = true;
         }
 
         if (tagExtra[kForScopeStartIndex]) {
@@ -342,7 +344,7 @@ export default {
     },
     dom: {
       enter(tag) {
-        if (tag.node.attributeTags.length) return;
+        if (tag.node.body.attributeTags) return;
 
         const tagBody = tag.get("body");
         const bodySection = getSectionForBody(tagBody);
@@ -360,7 +362,7 @@ export default {
         }
       },
       exit(tag) {
-        if (tag.node.attributeTags.length) return;
+        if (tag.node.body.attributeTags) return;
 
         const tagBody = tag.get("body");
         const tagSection = getSection(tag);
@@ -369,7 +371,7 @@ export default {
         const tagExtra = node.extra!;
         const { referencedBindings } = tagExtra;
         const nodeRef = isOnlyChildInParent(tag)
-          ? tag.parentPath.parent.extra![kNativeTagBinding]!
+          ? getParentTag(tag)!.node.extra![kNativeTagBinding]!
           : tag.node.extra![kForMarkerBinding]!;
         setSubscriberBuilder(tag, (signal: t.Expression) => {
           return callRuntime(
@@ -572,10 +574,8 @@ function isOnlyChildInParent(tag: t.NodePath<t.MarkoTag>) {
     return extra[kOnlyChildInParent];
   }
 
-  if (
-    t.isMarkoTag(tag.parentPath?.parent) &&
-    getTagDef(tag.parentPath!.parentPath! as t.NodePath<t.MarkoTag>)?.html
-  ) {
+  const parentTag = getParentTag(tag);
+  if (parentTag && getTagDef(parentTag)?.html) {
     return (extra[kOnlyChildInParent] =
       (tag.parent as t.MarkoTagBody).body.length === 1);
   }