Fix content treated as string after JSX comment inside expression (#895)

* add test * fix: html comment in component inside expression * fix typo in tests * test: add more printer tests * fix jsx inline and block comments in markup * remove unused helper functions * cleanup tests * simplify comment checking logic * Fix insertion mode switch when exiting expression * chore: add __debug_bin to .gitignore * Fix code in old printer test * tidy up code * revert fix of unrelated issue * chore: changeset * chore: update lazy changeset * Update .changeset/lazy-beers-develop.md Co-authored-by: Sarah Rainsberger <[email protected]> * move `peekIs` out on `RemoveComments` * refactor `RemoveComments` to only return a string --------- Co-authored-by: Sarah Rainsberger <[email protected]>
withastro · Dec 20, 2023 · 4f74c05 · 4f74c05
1 parent cef211b
commit 4f74c05
Show file tree

Hide file tree

Showing 9 changed files with 137 additions and 43 deletions.
diff --git a/.changeset/lazy-beers-develop.md b/.changeset/lazy-beers-develop.md
@@ -0,0 +1,5 @@
+---
+'@astrojs/compiler': patch
+---
+
+Fixes an issue where HTML and JSX comments lead to subsequent content being incorrectly treated as plain text when they have parent expressions.
diff --git a/.gitignore b/.gitignore
@@ -4,4 +4,5 @@ node_modules
 *.wasm
 /astro
 debug.test
+__debug_bin
 packages/compiler/sourcemap.mjs
diff --git a/internal/helpers/js_comment_utils.go b/internal/helpers/js_comment_utils.go
@@ -0,0 +1,45 @@
+package helpers
+
+import (
+	"strings"
+)
+
+func peekIs(input string, cur int, assert byte) bool {
+	return cur+1 < len(input) && input[cur+1] == assert
+}
+
+// RemoveComments removes both block and inline comments from a string
+func RemoveComments(input string) string {
+	var (
+		sb        = strings.Builder{}
+		inComment = false
+	)
+	for cur := 0; cur < len(input); cur++ {
+		if input[cur] == '/' && !inComment {
+			if peekIs(input, cur, '*') {
+				inComment = true
+				cur++
+			} else if peekIs(input, cur, '/') {
+				// Skip until the end of line for inline comments
+				for cur < len(input) && input[cur] != '\n' {
+					cur++
+				}
+				continue
+			}
+		} else if input[cur] == '*' && inComment && peekIs(input, cur, '/') {
+			inComment = false
+			cur++
+			continue
+		}
+
+		if !inComment {
+			sb.WriteByte(input[cur])
+		}
+	}
+
+	if inComment {
+		return ""
+	}
+
+	return strings.TrimSpace(sb.String())
+}
diff --git a/internal/parser.go b/internal/parser.go
@@ -2794,6 +2794,13 @@ func inExpressionIM(p *parser) bool {
 			p.im = textIM
 		}
 		return true
+	case CommentToken:
+		p.addChild(&Node{
+			Type: CommentNode,
+			Data: p.tok.Data,
+			Loc:  p.generateLoc(),
+		})
+		return true
 	}
 	p.im = p.originalIM
 	p.originalIM = nil

diff --git a/internal/printer/print-to-js.go b/internal/printer/print-to-js.go
@@ -13,6 +13,7 @@ import (
 
 	. "github.com/withastro/compiler/internal"
 	"github.com/withastro/compiler/internal/handler"
+	"github.com/withastro/compiler/internal/helpers"
 	"github.com/withastro/compiler/internal/js_scanner"
 	"github.com/withastro/compiler/internal/loc"
 	"github.com/withastro/compiler/internal/sourcemap"
@@ -88,13 +89,18 @@ func printToJs(p *printer, n *Node, cssLen int, opts transform.TransformOptions)
 const whitespace = " \t\r\n\f"
 
 // Returns true if the expression only contains a comment block (e.g. {/* a comment */})
-func expressionOnlyHasCommentBlock(n *Node) bool {
-	clean, _ := removeComments(n.FirstChild.Data)
-	return n.FirstChild.NextSibling == nil &&
+func expressionOnlyHasComment(n *Node) bool {
+	if n.FirstChild == nil {
+		return false
+	}
+	clean := helpers.RemoveComments(n.FirstChild.Data)
+	trimmedData := strings.TrimLeft(n.FirstChild.Data, whitespace)
+	result := n.FirstChild.NextSibling == nil &&
 		n.FirstChild.Type == TextNode &&
-		// removeComments iterates over text and most of the time we won't be parsing comments so lets check if text starts with /* before iterating
-		strings.HasPrefix(strings.TrimLeft(n.FirstChild.Data, whitespace), "/*") &&
+		// RemoveComments iterates over text and most of the time we won't be parsing comments so lets check if text starts with /* or // before iterating
+		(strings.HasPrefix(trimmedData, "/*") || strings.HasPrefix(trimmedData, "//")) &&
 		len(clean) == 0
+	return result
 }
 
 func emptyTextNodeWithoutSiblings(n *Node) bool {
@@ -312,7 +318,7 @@ func render1(p *printer, n *Node, opts RenderOptions) {
 	if n.Expression {
 		if n.FirstChild == nil {
 			p.print("${(void 0)")
-		} else if expressionOnlyHasCommentBlock(n) {
+		} else if expressionOnlyHasComment(n) {
 			// we do not print expressions that only contain comment blocks
 			return
 		} else {
@@ -624,9 +630,12 @@ func render1(p *printer, n *Node, opts RenderOptions) {
 						}
 					}
 
-					// Only slot ElementNodes or non-empty TextNodes!
-					// CommentNode and others should not be slotted
-					if c.Type == ElementNode || (c.Type == TextNode && !emptyTextNodeWithoutSiblings(c)) {
+					// Only slot ElementNodes (except expressions containing only comments) or non-empty TextNodes!
+					// CommentNode, JSX comments and others should not be slotted
+					if expressionOnlyHasComment(c) {
+						continue
+					}
+					if c.Type == ElementNode || c.Type == TextNode && !emptyTextNodeWithoutSiblings(c) {
 						slottedChildren[slotProp] = append(slottedChildren[slotProp], c)
 					}
 				}

diff --git a/internal/printer/print-to-tsx.go b/internal/printer/print-to-tsx.go
@@ -8,6 +8,7 @@ import (
 	. "github.com/withastro/compiler/internal"
 	astro "github.com/withastro/compiler/internal"
 	"github.com/withastro/compiler/internal/handler"
+	"github.com/withastro/compiler/internal/helpers"
 	"github.com/withastro/compiler/internal/js_scanner"
 	"github.com/withastro/compiler/internal/loc"
 	"github.com/withastro/compiler/internal/sourcemap"
@@ -229,7 +230,7 @@ declare const Astro: Readonly<import('astro').AstroGlobal<%s, typeof %s`, propsI
 		p.addSourceMapping(n.Loc[0])
 		if n.FirstChild == nil {
 			p.print("{(void 0)")
-		} else if expressionOnlyHasCommentBlock(n) {
+		} else if expressionOnlyHasComment(n) {
 			// we do not print expressions that only contain comment blocks
 			return
 		} else {
@@ -350,7 +351,7 @@ declare const Astro: Readonly<import('astro').AstroGlobal<%s, typeof %s`, propsI
 			p.print("}")
 			endLoc = a.KeyLoc.Start + len(a.Key) + 1
 		case astro.ShorthandAttribute:
-			withoutComments, _ := removeComments(a.Key)
+			withoutComments := helpers.RemoveComments(a.Key)
 			if len(withoutComments) == 0 {
 				return
 			}
@@ -421,7 +422,7 @@ declare const Astro: Readonly<import('astro').AstroGlobal<%s, typeof %s`, propsI
 		case astro.SpreadAttribute:
 			// noop
 		case astro.ShorthandAttribute:
-			withoutComments, _ := removeComments(a.Key)
+			withoutComments := helpers.RemoveComments(a.Key)
 			if len(withoutComments) == 0 {
 				return
 			}

diff --git a/internal/printer/printer.go b/internal/printer/printer.go
@@ -8,6 +8,7 @@ import (
 
 	astro "github.com/withastro/compiler/internal"
 	"github.com/withastro/compiler/internal/handler"
+	"github.com/withastro/compiler/internal/helpers"
 	"github.com/withastro/compiler/internal/js_scanner"
 	"github.com/withastro/compiler/internal/loc"
 	"github.com/withastro/compiler/internal/sourcemap"
@@ -337,7 +338,7 @@ func (p *printer) printAttributesToObject(n *astro.Node) {
 			p.addSourceMapping(loc.Loc{Start: a.KeyLoc.Start - 3})
 			p.print(`...(` + strings.TrimSpace(a.Key) + `)`)
 		case astro.ShorthandAttribute:
-			withoutComments, _ := removeComments(a.Key)
+			withoutComments := helpers.RemoveComments(a.Key)
 			if len(withoutComments) == 0 {
 				lastAttributeSkipped = true
 				continue
@@ -422,7 +423,7 @@ func (p *printer) printAttribute(attr astro.Attribute, n *astro.Node) {
 			p.printf(`,undefined,{"class":"astro-%s"})}`, p.opts.Scope)
 		}
 	case astro.ShorthandAttribute:
-		withoutComments, _ := removeComments(attr.Key)
+		withoutComments := helpers.RemoveComments(attr.Key)
 		if len(withoutComments) == 0 {
 			return
 		}

diff --git a/internal/printer/printer_test.go b/internal/printer/printer_test.go
@@ -832,6 +832,20 @@ const groups = [[0, 1, 2], [3, 4, 5]];
 				code: "${$$maybeRenderHead($$result)}<body><!-- \\`npm install astro\\` --></body>",
 			},
 		},
+		{
+			name:   "HTML comment in component inside expression I",
+			source: "{(() => <Component><!--Hi--></Component>)}",
+			want: want{
+				code: "${(() => $$render`${$$renderComponent($$result,'Component',Component,{},{})}`)}",
+			},
+		},
+		{
+			name:   "HTML comment in component inside expression II",
+			source: "{list.map(() => <Component><!--Hi--></Component>)}",
+			want: want{
+				code: "${list.map(() => $$render`${$$renderComponent($$result,'Component',Component,{},{})}`)}",
+			},
+		},
 		{
 			name:   "nested expressions",
 			source: `<article>{(previous || next) && <aside>{previous && <div>Previous Article: <a rel="prev" href={new URL(previous.link, Astro.site).pathname}>{previous.text}</a></div>}{next && <div>Next Article: <a rel="next" href={new URL(next.link, Astro.site).pathname}>{next.text}</a></div>}</aside>}</article>`,
@@ -1010,7 +1024,7 @@ const name = "world";
 			},
 		},
 		{
-			name: "head expression and conditional renderin of fragment",
+			name: "head expression and conditional rendering of fragment",
 			source: `---
 const testBool = true;
 ---
@@ -2826,12 +2840,50 @@ const items = ["Dog", "Cat", "Platipus"];
 			},
 		},
 		{
-			name:   "comment only expressions are removed",
+			name:   "comment only expressions are removed I",
 			source: `{/* a comment 1 */}<h1>{/* a comment 2*/}Hello</h1>`,
 			want: want{
 				code: `${$$maybeRenderHead($$result)}<h1>Hello</h1>`,
 			},
 		},
+		{
+			name: "comment only expressions are removed II",
+			source: `{
+    list.map((i) => (
+        <Component>
+            {
+                // hello
+            }
+        </Component>
+    ))
+}`,
+			want: want{
+				code: `${
+    list.map((i) => (
+        $$render` + BACKTICK + `${$$renderComponent($$result,'Component',Component,{},{})}` + BACKTICK + `
+    ))
+}`,
+			},
+		},
+		{
+			name: "comment only expressions are removed III",
+			source: `{
+    list.map((i) => (
+        <Component>
+            {
+                /* hello */
+            }
+        </Component>
+    ))
+}`,
+			want: want{
+				code: `${
+    list.map((i) => (
+        $$render` + BACKTICK + `${$$renderComponent($$result,'Component',Component,{},{})}` + BACKTICK + `
+    ))
+}`,
+			},
+		},
 		{
 			name:   "component with only a script",
 			source: "<script>console.log('hello world');</script>",

diff --git a/internal/printer/utils.go b/internal/printer/utils.go
@@ -1,7 +1,6 @@
 package printer
 
 import (
-	"errors"
 	"fmt"
 	"regexp"
 	"strings"
@@ -95,32 +94,6 @@ func encodeDoubleQuote(str string) string {
 	return strings.Replace(str, `"`, "&quot;", -1)
 }
 
-// Remove comment blocks from string (e.g. "/* a comment */aProp" => "aProp")
-func removeComments(input string) (string, error) {
-	var (
-		sb        = strings.Builder{}
-		inComment = false
-	)
-	for cur := 0; cur < len(input); cur++ {
-		peekIs := func(assert byte) bool { return cur+1 < len(input) && input[cur+1] == assert }
-		if input[cur] == '/' && !inComment && peekIs('*') {
-			inComment = true
-			cur++
-		} else if input[cur] == '*' && inComment && peekIs('/') {
-			inComment = false
-			cur++
-		} else if !inComment {
-			sb.WriteByte(input[cur])
-		}
-	}
-
-	if inComment {
-		return "", errors.New("unterminated comment")
-	}
-
-	return strings.TrimSpace(sb.String()), nil
-}
-
 func convertAttributeValue(n *astro.Node, attrName string) string {
 	expr := `""`
 	if transform.HasAttr(n, attrName) {