Caching (#44)

* fix: don't cache call expressions unless event handlers

* fix: cache function calls

* chore: add changeset

.changeset/new-avocados-walk.md

@@ -0,0 +1,5 @@
+---
+"@marko/tags-api-preview": patch
+---
+
+improve member expression caching

src/__tests__/fixtures/const/__snapshots__/const-state-derived-from-prototype-function/node.render.expected.html

@@ -0,0 +1,6 @@
+<p>
+  9 string length = 1
+</p>
+<button>
+  increment
+</button>

src/__tests__/fixtures/const/__snapshots__/const-state-derived-from-prototype-function/web.render.expected.html

@@ -0,0 +1,6 @@
+<p>
+  9 string length = 1
+</p>
+<button>
+  increment
+</button>

src/__tests__/fixtures/const/__snapshots__/const-state-derived-from-prototype-function/web.step-0.expected.html

@@ -0,0 +1,6 @@
+<p>
+  10 string length = 2
+</p>
+<button>
+  increment
+</button>

src/__tests__/fixtures/const/index.test.ts

@@ -21,6 +21,16 @@ describe(
   ])
 );
 
+describe(
+  "<const> state derived from prototype function",
+  fixture("./templates/state-derived-prototype-function.marko", [
+    { value: 1 },
+    async ({ screen, fireEvent }) => {
+      await fireEvent.click(screen.getByText("increment"));
+    },
+  ])
+);
+
 describe(
   "<const> assignment error",
   fixture("./templates/error-assignment.marko")

src/__tests__/fixtures/const/templates/state-derived-prototype-function.marko

@@ -0,0 +1,4 @@
+<let/value=9/>
+<const/length=value.toString().length/>
+<p>${value} string length = ${length}</p>
+<button onClick=(() => value++)>increment</button>

src/__tests__/fixtures/misc/caching/__snapshots__/cached/function-call/node.render.expected.html

@@ -0,0 +1,3 @@
+<button>
+  increment
+</button>

src/__tests__/fixtures/misc/caching/__snapshots__/cached/function-call/web.render.expected.html

@@ -0,0 +1,3 @@
+<button>
+  increment
+</button>

src/__tests__/fixtures/misc/caching/__snapshots__/cached/function-call/web.step-0.expected.html

@@ -0,0 +1,3 @@
+<button>
+  increment
+</button>

src/__tests__/fixtures/misc/caching/index.test.ts

@@ -0,0 +1,51 @@
+import { resetHistory, spy } from "sinon";
+import fixture, { FixtureHelpers } from "../../../fixture";
+const increment = click("Increment");
+const toggle = click("Toggle");
+
+const onRender = spy();
+
+describe("cached", () => {
+  beforeEach(() => resetHistory());
+
+  describe(
+    "conditional value",
+    fixture("./templates/conditional-value.marko", [
+      {},
+      increment,
+      toggle,
+      toggle,
+      increment,
+      toggle,
+    ])
+  );
+
+  describe(
+    "function call",
+    fixture("./templates/function-call.marko", [
+      { fn: onRender },
+      async ({ expect, screen, fireEvent, rerender }) => {
+        expect(onRender).calledOnce;
+        resetHistory();
+        expect(onRender).not.called;
+
+        await fireEvent.click(screen.getByText("increment"));
+        expect(onRender).not.called;
+        resetHistory();
+
+        await rerender();
+        expect(onRender).not.called;
+        resetHistory();
+
+        await rerender({ fn: () => onRender() });
+        expect(onRender).calledOnce;
+        resetHistory();
+      },
+    ])
+  );
+});
+
+function click(text: string) {
+  return async ({ fireEvent, screen }: FixtureHelpers) =>
+    await fireEvent.click(screen.getByText(text));
+}

src/__tests__/fixtures/misc/caching/templates/function-call.marko

@@ -0,0 +1,3 @@
+<let/count=0/>
+<button onClick=(() => count++)>increment</button>
+<const/_=input.fn()/>

src/transform/cached-values/transform.ts

@@ -10,12 +10,19 @@ interface Deps {
 }
 
 interface DepsVisitorState {
+  parent: t.Node;
+  node: t.Node;
   parentTag: t.NodePath;
   deps: Deps | undefined;
   shouldCache: boolean;
 }
 
 const depsVisitor = {
+  enter(path, state) {
+    if (path.parent === state.parent && path.node !== state.node) {
+      path.skip();
+    }
+  },
   Function(_, state) {
     state.shouldCache = true;
   },
@@ -55,22 +62,25 @@ const depsVisitor = {
         const curDeps = deps[name];
         if (curDeps === true) return;
 
-        if (
-          parent.isMemberExpression() &&
-          (!parent.node.computed ||
-            isStringOrNumericLiteral(parent.node.property.type))
-        ) {
-          const prop = parent.node.property as
-            | t.Identifier
-            | t.StringLiteral
-            | t.NumericLiteral;
-          parent = parent.parentPath;
-          deps = curDeps || (deps[name] = {} as Deps);
-          name = prop.type === "Identifier" ? prop.name : prop.value + "";
-        } else {
-          deps[name] = true;
-          return;
+        if (parent.isMemberExpression()) {
+          const literalName = getPropertyNameLiteral(parent.node);
+          if (
+            literalName !== undefined &&
+            !(
+              parent.parentPath.isCallExpression() &&
+              parent.parentPath.node.callee === parent.node &&
+              !isEventHandlerName(literalName)
+            )
+          ) {
+            parent = parent.parentPath;
+            deps = curDeps || (deps[name] = {} as Deps);
+            name = literalName;
+            continue;
+          }
         }
+
+        deps[name] = true;
+        return;
         // eslint-disable-next-line no-constant-condition
       } while (true);
     }
@@ -114,19 +124,22 @@ export default {
 
 function cacheExprIfNeeded(
   parentTag: t.NodePath<t.MarkoTag>,
-  expr: t.NodePath<any>
+  exprPath: t.NodePath<any>
 ) {
+  const parentPath = exprPath.parentPath!;
   const state: DepsVisitorState = {
     parentTag,
+    node: exprPath.node,
+    parent: parentPath.node,
     deps: undefined,
     shouldCache: false,
   };
-  expr.traverse(depsVisitor, state);
+  parentPath.traverse(depsVisitor, state);
 
   if (state.shouldCache) {
-    const { file } = expr.hub;
+    const { file } = exprPath.hub;
     const { component } = ensureLifecycle(parentTag)!;
-    expr
+    exprPath
       .replaceWith(
         t.callExpression(
           importRuntimeNamed(file, "transform/cached-values", "cache"),
@@ -140,7 +153,7 @@ function cacheExprIfNeeded(
                   t.arrayExpression(state.deps ? toDepsArray(state.deps) : []),
                 ]
               ),
-              expr.node
+              exprPath.node
             ),
           ]
         )
@@ -174,6 +187,19 @@ function* getDefaultExpressions(
   }
 }
 
+function getPropertyNameLiteral(memberExpression: t.MemberExpression) {
+  if (memberExpression.computed) {
+    if (isStringOrNumericLiteral(memberExpression.property.type)) {
+      return (
+        (memberExpression.property as t.StringLiteral | t.NumericLiteral)
+          .value + ""
+      );
+    }
+  } else if (memberExpression.property.type === "Identifier") {
+    return memberExpression.property.name;
+  }
+}
+
 function isStringOrNumericLiteral(type: string) {
   switch (type) {
     case "StringLiteral":
@@ -184,6 +210,10 @@ function isStringOrNumericLiteral(type: string) {
   }
 }
 
+function isEventHandlerName(name: string) {
+  return /^on[A-Z]/.test(name);
+}
+
 function toDepsArray(
   deps: Deps,
   object?: t.Expression,