fix: comment printing within expressions

marko-js · May 9, 2023 · ac2a1ef · ac2a1ef
1 parent 7a21d27
commit ac2a1ef
Show file tree

Hide file tree

Showing 16 changed files with 141 additions and 62 deletions.
diff --git a/src/__tests__/__snapshots__/attr-with-comment-short.expected/auto.marko b/src/__tests__/__snapshots__/attr-with-comment-short.expected/auto.marko
@@ -1,4 +1,6 @@
-<custom-tag id="hello"/>
+<custom-tag id=(
+  "hello" // this comment should not be removed removed
+)/>
 <!-- <custom-tag
   id="hello" // this comment is currently not supported
 /> -->
diff --git a/src/__tests__/__snapshots__/attr-with-comment-short.expected/html.marko b/src/__tests__/__snapshots__/attr-with-comment-short.expected/html.marko
@@ -1,4 +1,6 @@
-<custom-tag id="hello"/>
+<custom-tag id=(
+  "hello" // this comment should not be removed removed
+)/>
 <!-- <custom-tag
   id="hello" // this comment is currently not supported
 /> -->
diff --git a/src/__tests__/__snapshots__/attr-with-comment-short.expected/with-parens.marko b/src/__tests__/__snapshots__/attr-with-comment-short.expected/with-parens.marko
@@ -1,4 +1,6 @@
-<custom-tag id="hello"/>
+<custom-tag id=(
+  "hello" // this comment should not be removed removed
+)/>
 <!-- <custom-tag
   id="hello" // this comment is currently not supported
 /> -->
diff --git a/src/__tests__/__snapshots__/attr-with-newlines.expected/auto.marko b/src/__tests__/__snapshots__/attr-with-newlines.expected/auto.marko
@@ -9,9 +9,9 @@
 )/>
 <div data-current-text=(
   current &&
-    current
-      .replace("{page_number}", "{item_number}")
-      .replace("{current_page}", "{item_number}")
+  current
+    .replace("{page_number}", "{item_number}")
+    .replace("{current_page}", "{item_number}")
 )/>
 
 <div
@@ -26,8 +26,8 @@
   )
   data-current-text=(
     current &&
-      current
-        .replace("{page_number}", "{item_number}")
-        .replace("{current_page}", "{item_number}")
+    current
+      .replace("{page_number}", "{item_number}")
+      .replace("{current_page}", "{item_number}")
   )
 />
diff --git a/src/__tests__/__snapshots__/attr-with-newlines.expected/concise.marko b/src/__tests__/__snapshots__/attr-with-newlines.expected/concise.marko
@@ -9,9 +9,9 @@ div class=(
 )
 div data-current-text=(
   current &&
-    current
-      .replace("{page_number}", "{item_number}")
-      .replace("{current_page}", "{item_number}")
+  current
+    .replace("{page_number}", "{item_number}")
+    .replace("{current_page}", "{item_number}")
 )
 
 div [
@@ -26,8 +26,8 @@ div [
   )
   data-current-text=(
     current &&
-      current
-        .replace("{page_number}", "{item_number}")
-        .replace("{current_page}", "{item_number}")
+    current
+      .replace("{page_number}", "{item_number}")
+      .replace("{current_page}", "{item_number}")
   )
 ]
diff --git a/src/__tests__/__snapshots__/attr-with-newlines.expected/html.marko b/src/__tests__/__snapshots__/attr-with-newlines.expected/html.marko
@@ -9,9 +9,9 @@
 )/>
 <div data-current-text=(
   current &&
-    current
-      .replace("{page_number}", "{item_number}")
-      .replace("{current_page}", "{item_number}")
+  current
+    .replace("{page_number}", "{item_number}")
+    .replace("{current_page}", "{item_number}")
 )/>
 
 <div
@@ -26,8 +26,8 @@
   )
   data-current-text=(
     current &&
-      current
-        .replace("{page_number}", "{item_number}")
-        .replace("{current_page}", "{item_number}")
+    current
+      .replace("{page_number}", "{item_number}")
+      .replace("{current_page}", "{item_number}")
   )
 />
diff --git a/src/__tests__/__snapshots__/attr-with-newlines.expected/with-parens.marko b/src/__tests__/__snapshots__/attr-with-newlines.expected/with-parens.marko
@@ -9,9 +9,9 @@
 )/>
 <div data-current-text=(
   current &&
-    current
-      .replace("{page_number}", "{item_number}")
-      .replace("{current_page}", "{item_number}")
+  current
+    .replace("{page_number}", "{item_number}")
+    .replace("{current_page}", "{item_number}")
 )/>
 
 <div
@@ -26,8 +26,8 @@
   )
   data-current-text=(
     current &&
-      current
-        .replace("{page_number}", "{item_number}")
-        .replace("{current_page}", "{item_number}")
+    current
+      .replace("{page_number}", "{item_number}")
+      .replace("{current_page}", "{item_number}")
   )
 />
diff --git a/src/__tests__/__snapshots__/expression-comments.expected/auto.marko b/src/__tests__/__snapshots__/expression-comments.expected/auto.marko
@@ -0,0 +1,11 @@
+class {
+  // Comments in a class
+  aFunction() {
+    // Test comment
+  }
+}
+
+<div data-test=(
+  // This was important
+  "test"
+)/>
diff --git a/src/__tests__/__snapshots__/expression-comments.expected/concise.marko b/src/__tests__/__snapshots__/expression-comments.expected/concise.marko
@@ -0,0 +1,11 @@
+class {
+  // Comments in a class
+  aFunction() {
+    // Test comment
+  }
+}
+
+div data-test=(
+  // This was important
+  "test"
+)
diff --git a/src/__tests__/__snapshots__/expression-comments.expected/html.marko b/src/__tests__/__snapshots__/expression-comments.expected/html.marko
@@ -0,0 +1,11 @@
+class {
+  // Comments in a class
+  aFunction() {
+    // Test comment
+  }
+}
+
+<div data-test=(
+  // This was important
+  "test"
+)/>
diff --git a/src/__tests__/__snapshots__/expression-comments.expected/with-parens.marko b/src/__tests__/__snapshots__/expression-comments.expected/with-parens.marko
@@ -0,0 +1,11 @@
+class {
+  // Comments in a class
+  aFunction() {
+    // Test comment
+  }
+}
+
+<div data-test=(
+  // This was important
+  "test"
+)/>
diff --git a/src/__tests__/fixtures/attr-with-comment-short.marko b/src/__tests__/fixtures/attr-with-comment-short.marko
@@ -1,5 +1,5 @@
 <custom-tag id=(
-  "hello" // this comment is removed, it probably should not be
+  "hello" // this comment should not be removed removed
 )></custom-tag>
 
 <!-- <custom-tag

diff --git a/src/__tests__/fixtures/expression-comments.marko b/src/__tests__/fixtures/expression-comments.marko
@@ -0,0 +1,11 @@
+class {
+    // Comments in a class
+    aFunction() {
+        // Test comment
+    }
+}
+
+<div data-test=(
+    // This was important
+    "test"
+)/>
diff --git a/src/index.ts b/src/index.ts
@@ -37,8 +37,16 @@ const { builders: b, utils } = doc;
 const identity = <T>(val: T) => val;
 const embeddedPlaceholderReg = /__EMBEDDED_PLACEHOLDER_(\d+)__/g;
 const expressionParser: CustomParser = (code, parsers, options) => {
-  const ast = parsers["babel-ts"](`(${code});`, options);
-  return { ...ast, program: ast.program.body[0].expression };
+  const ast = parsers["babel-ts"](code, options);
+  const { tokens, comments, range } = ast;
+  const node = ast.program.body[0].expression;
+  return {
+    type: "JsExpressionRoot",
+    tokens,
+    comments,
+    node,
+    range,
+  };
 };
 
 export const languages: SupportLanguage[] = [
@@ -693,7 +701,7 @@ export const printers: Record<string, Printer<Node>> = {
             }
             case "params": {
               return tryPrintEmbed(
-                `(${node.code})=>_`,
+                toExpression(`(${node.code})=>_`),
                 expressionParser,
                 (doc: any) => {
                   const { contents } = doc.contents[0];
@@ -722,7 +730,7 @@ export const printers: Record<string, Printer<Node>> = {
 
         case "MarkoClass":
           return (toDoc as any)(
-            `class ${getOriginalCodeForNode(opts, node.body)}`,
+            toExpression(`class ${getOriginalCodeForNode(opts, node.body)}`),
             { parser: expressionParser },
             { stripTrailingHardline: true }
           );
@@ -742,7 +750,7 @@ export const printers: Record<string, Printer<Node>> = {
         );
       } else {
         return tryPrintEmbed(
-          getOriginalCodeForNode(opts, node),
+          toExpression(getOriginalCodeForNode(opts, node)),
           expressionParser
         );
       }
@@ -802,3 +810,7 @@ function replaceEmbeddedPlaceholders(doc: Doc, placeholders: Doc[]) {
     return cur;
   });
 }
+
+function toExpression(code: string) {
+  return `(${code}\n)`;
+}
diff --git a/src/utils/get-original-code.ts b/src/utils/get-original-code.ts
@@ -5,9 +5,6 @@ import babelGenerator from "@babel/generator";
 const generate = (babelGenerator as any).default || babelGenerator;
 
 export function getOriginalCodeForNode(opts: ParserOptions<Node>, node: Node) {
-  const literal = literalToString(node);
-  if (literal !== undefined) return literal;
-
   const loc = node.loc;
   if (!loc) {
     return generate(node as any, {
@@ -18,10 +15,30 @@ export function getOriginalCodeForNode(opts: ParserOptions<Node>, node: Node) {
     }).code;
   }
 
-  return opts.originalText.slice(
-    locToPos(loc.start, opts),
-    locToPos(loc.end, opts)
-  );
+  let start = loc.start;
+  if (node.leadingComments?.length) {
+    const commentStart = node.leadingComments[0].loc.start;
+    if (
+      commentStart.line < start.line ||
+      (commentStart.line === start.line && commentStart.column < start.column)
+    ) {
+      start = commentStart;
+    }
+  }
+
+  let end = loc.end;
+  if (node.trailingComments?.length) {
+    const commentEnd =
+      node.trailingComments[node.trailingComments.length - 1].loc.end;
+    if (
+      commentEnd.line > end.line ||
+      (commentEnd.line === end.line && commentEnd.column > end.column)
+    ) {
+      end = commentEnd;
+    }
+  }
+
+  return opts.originalText.slice(locToPos(start, opts), locToPos(end, opts));
 }
 
 export function getOriginalCodeForList(
@@ -31,16 +48,3 @@ export function getOriginalCodeForList(
 ) {
   return list.map((node) => getOriginalCodeForNode(opts, node)).join(sep);
 }
-
-function literalToString(node: Node) {
-  switch (node.type) {
-    case "StringLiteral":
-      return `"${node.value.replace(/(["\\])/g, "\\$1")}"`;
-    case "NumericLiteral":
-      return node.value.toString();
-    case "BooleanLiteral":
-      return node.value ? "true" : "false";
-    case "NullLiteral":
-      return "null";
-  }
-}
diff --git a/src/utils/with-parens-if-needed.ts b/src/utils/with-parens-if-needed.ts
@@ -11,15 +11,17 @@ export default function withParensIfNeeded(
   getValDoc: () => Doc
 ) {
   if (
-    !enclosedNodeTypeReg.test(node.type) &&
-    outerCodeMatches(
-      doc.printer.printDocToString(getValDoc(), {
-        ...opts,
-        printWidth: 0,
-      }).formatted,
-      /\s|>/y,
-      opts.markoAttrParen
-    )
+    (node as any).leadingComments?.length ||
+    (node as any).trailingComments?.length ||
+    (!enclosedNodeTypeReg.test(node.type) &&
+      outerCodeMatches(
+        doc.printer.printDocToString(getValDoc(), {
+          ...opts,
+          printWidth: 0,
+        }).formatted,
+        /\s|>/y,
+        opts.markoAttrParen
+      ))
   ) {
     return ["(", b.indent([b.softline, getValDoc()]), b.softline, ")"];
   }