From 60e097f0ec9d01f8d105dd7adf2a87f233dc3cd7 Mon Sep 17 00:00:00 2001
From: Michael Rawlings <mirawlings@ebay.com>
Date: Tue, 2 Apr 2024 17:28:35 -0400
Subject: [PATCH] feat: hoist isEffect when merging references, refactor native
 tag analyze

---
 .../translator-tags/src/util/references.ts    | 30 ++++++++++++-------
 .../src/visitors/tag/native-tag.ts            | 23 ++++++--------
 2 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/packages/translator-tags/src/util/references.ts b/packages/translator-tags/src/util/references.ts
index db1c7ade5..02266055a 100644
--- a/packages/translator-tags/src/util/references.ts
+++ b/packages/translator-tags/src/util/references.ts
@@ -318,21 +318,31 @@ export function finalizeReferences() {
   if (mergedReferences.size) {
     for (const [target, nodes] of mergedReferences) {
       const targetExtra = (target.node.extra ??= {});
-      let newReferences: ReferencedBindings = targetExtra.referencedBindings;
+      let { referencedBindings, isEffect } = targetExtra;
       for (const node of nodes) {
         const extra = node?.extra;
-        const referencedBindings = extra?.referencedBindings;
-        if (referencedBindings) {
-          newReferences = bindingUtil.union(newReferences, referencedBindings);
-          forEach(referencedBindings, ({ downstreamExpressions }) => {
-            downstreamExpressions.delete(extra);
-            downstreamExpressions.add(targetExtra);
-          });
+        if (extra) {
+          const additionalBindings = extra.referencedBindings;
+          isEffect ||= extra.isEffect;
+          if (additionalBindings) {
+            referencedBindings = bindingUtil.union(
+              referencedBindings,
+              additionalBindings,
+            );
+            forEach(additionalBindings, ({ downstreamExpressions }) => {
+              downstreamExpressions.delete(extra);
+              downstreamExpressions.add(targetExtra);
+            });
+          }
         }
       }
 
-      newReferences = findReferences(getOrCreateSection(target), newReferences);
-      targetExtra.referencedBindings = newReferences;
+      referencedBindings = findReferences(
+        getOrCreateSection(target),
+        referencedBindings,
+      );
+      targetExtra.referencedBindings = referencedBindings;
+      targetExtra.isEffect = isEffect;
     }
 
     mergedReferences.clear();
diff --git a/packages/translator-tags/src/visitors/tag/native-tag.ts b/packages/translator-tags/src/visitors/tag/native-tag.ts
index 4962eea17..970aaa3e8 100644
--- a/packages/translator-tags/src/visitors/tag/native-tag.ts
+++ b/packages/translator-tags/src/visitors/tag/native-tag.ts
@@ -26,11 +26,9 @@ import { currentProgramPath, scopeIdentifier } from "../program";
 
 export const kNativeTagBinding = Symbol("native tag binding");
 export const kSerializeMarker = Symbol("serialize marker");
-const kHasEventHandlers = Symbol("has event handlers");
 declare module "@marko/compiler/dist/types" {
   export interface NodeExtra {
     [kNativeTagBinding]?: Binding;
-    [kHasEventHandlers]?: boolean;
     [kSerializeMarker]?: boolean;
   }
 }
@@ -40,8 +38,8 @@ export default {
     enter(tag: t.NodePath<t.MarkoTag>) {
       const { node } = tag;
       const attrs = tag.get("attributes");
-      let section = tag.has("var") ? getOrCreateSection(tag) : undefined;
       let hasEventHandlers = false;
+      let hasDynamicAttributes = false;
 
       /**
        * The reason this seems like it does more work than it needs to
@@ -50,31 +48,30 @@ export default {
        */
       for (const attr of attrs) {
         if (isSpreadAttr(attr)) {
-          section ??= getOrCreateSection(tag);
+          (attr.node.value.extra ??= {}).isEffect = true;
+          hasEventHandlers = true;
+          hasDynamicAttributes = true;
           mergeReferences(
             tag,
             attrs.map((attr) => attr.node.value),
           );
-          hasEventHandlers = true;
-          // TODO: should add isEffect to all attributes in a spread?
-          break;
         } else if (isEventHandler((attr.node as t.MarkoAttribute).name)) {
           (attr.node.value.extra ??= {}).isEffect = true;
-          section ??= getOrCreateSection(tag);
           hasEventHandlers = true;
         } else if (!evaluate(attr).confident) {
-          section ??= getOrCreateSection(tag);
+          hasDynamicAttributes = true;
         }
       }
 
-      if (section !== undefined) {
+      if (tag.has("var") || hasEventHandlers || hasDynamicAttributes) {
         currentProgramPath.node.extra.isInteractive ||= hasEventHandlers;
+        const section = getOrCreateSection(tag);
         const tagName =
           node.name.type === "StringLiteral"
             ? node.name.value
             : t.toIdentifier(tag.get("name"));
         const tagExtra = (node.extra ??= {});
-        tagExtra[kHasEventHandlers] = hasEventHandlers;
+        tagExtra[kSerializeMarker] = tag.has("var") || hasEventHandlers;
         tagExtra[kNativeTagBinding] = createBinding(
           "#" + tagName,
           BindingType.dom,
@@ -318,9 +315,7 @@ export default {
 
       if (
         nodeRef &&
-        (extra[kHasEventHandlers] ||
-          extra[kSerializeMarker] ||
-          tag.has("var") ||
+        (extra[kSerializeMarker] ||
           tag.node.attributes.some((attr) =>
             isStatefulReferences(attr.value.extra?.referencedBindings),
           ))