fix #3605: print the original JSX AST unmodified

evanw · evanw · commit e04a6900b765 · 2024-01-23T01:16:05.000-05:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,30 @@
 
 ## Unreleased
 
+* The "preserve" JSX mode now preserves JSX text verbatim ([#3605](https://github.com/evanw/esbuild/issues/3605))
+
+    The [JSX specification](https://facebook.github.io/jsx/) deliberately doesn't specify how JSX text is supposed to be interpreted and there is no canonical way to interpret JSX text. Two most popular interpretations are Babel and TypeScript. Yes [they are different](https://twitter.com/jarredsumner/status/1456118847937781764) (esbuild [deliberately follows TypeScript](https://twitter.com/evanwallace/status/1456122279453208576) by the way). Previously esbuild normalized text to the TypeScript interpretation when the "preserve" JSX mode is active. However, "preserve" should arguably reproduce the original JSX text verbatim so that whatever JSX transform runs after esbuild is free to interpret it however it wants. So with this release, esbuild will now pass JSX text through unmodified:
+
+    ```jsx
+    // Original code
+    let el =
+      <a href={'/'} title='&apos;&quot;'> some text
+        {foo}
+          more text </a>
+
+    // Old output (with --loader=jsx --jsx=preserve)
+    let el = <a href="/" title={`'"`}>
+      {" some text"}
+      {foo}
+      {"more text "}
+    </a>;
+
+    // New output (with --loader=jsx --jsx=preserve)
+    let el = <a href={"/"} title='&apos;&quot;'> some text
+        {foo}
+          more text </a>;
+    ```
+
 * Allow JSX elements as JSX attribute values
 
     JSX has an obscure feature where you can use JSX elements in attribute position without surrounding them with `{...}`. It looks like this:
diff --git a/internal/bundler_tests/snapshots/snapshots_default.txt b/internal/bundler_tests/snapshots/snapshots_default.txt
@@ -711,48 +711,24 @@ console.log(
     x
   }</>,
   // Comments on absent AST nodes
-  <div>
-    {"before"}
-    {}
-    {"after"}
-  </div>,
-  <div>
-    {"before"}
-    {
-      /* comment 1 */
-      /* comment 2 */
-    }
-    {"after"}
-  </div>,
-  <div>
-    {"before"}
-    {
-      // comment 1
-      // comment 2
-    }
-    {"after"}
-  </div>,
-  <>
-    {"before"}
-    {}
-    {"after"}
-  </>,
-  <>
-    {"before"}
-    {
-      /* comment 1 */
-      /* comment 2 */
-    }
-    {"after"}
-  </>,
-  <>
-    {"before"}
-    {
-      // comment 1
-      // comment 2
-    }
-    {"after"}
-  </>
+  <div>before{}after</div>,
+  <div>before{
+    /* comment 1 */
+    /* comment 2 */
+  }after</div>,
+  <div>before{
+    // comment 1
+    // comment 2
+  }after</div>,
+  <>before{}after</>,
+  <>before{
+    /* comment 1 */
+    /* comment 2 */
+  }after</>,
+  <>before{
+    // comment 1
+    // comment 2
+  }after</>
 );
 
 ================================================================================
diff --git a/internal/js_ast/js_ast.go b/internal/js_ast/js_ast.go
@@ -449,6 +449,7 @@ func (*EImportIdentifier) isExpr()     {}
 func (*EPrivateIdentifier) isExpr()    {}
 func (*ENameOfSymbol) isExpr()         {}
 func (*EJSXElement) isExpr()           {}
+func (*EJSXText) isExpr()              {}
 func (*EMissing) isExpr()              {}
 func (*ENumber) isExpr()               {}
 func (*EBigInt) isExpr()               {}
@@ -770,6 +771,16 @@ type EJSXElement struct {
 	IsTagSingleLine bool
 }
 
+// The JSX specification doesn't say how JSX text is supposed to be interpreted
+// so our "preserve" JSX transform should reproduce the original source code
+// verbatim. One reason why this matters is because there is no canonical way
+// to interpret JSX text (Babel and TypeScript differ in what newlines mean).
+// Another reason is that some people want to do custom things such as this:
+// https://github.com/evanw/esbuild/issues/3605
+type EJSXText struct {
+	Raw string
+}
+
 type ENumber struct{ Value float64 }
 
 type EBigInt struct{ Value string }
diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go
@@ -5043,12 +5043,17 @@ func (p *parser) parseJSXElement(loc logger.Loc) js_ast.Expr {
 						if p.lexer.PreviousBackslashQuoteInJSX.Loc.Start > stringLoc.Start {
 							previousStringWithBackslashLoc = stringLoc
 						}
-						value = js_ast.Expr{Loc: stringLoc, Data: &js_ast.EString{Value: p.lexer.StringLiteral()}}
+						if p.options.jsx.Preserve {
+							value = js_ast.Expr{Loc: stringLoc, Data: &js_ast.EJSXText{Raw: p.lexer.Raw()}}
+						} else {
+							value = js_ast.Expr{Loc: stringLoc, Data: &js_ast.EString{Value: p.lexer.StringLiteral()}}
+						}
 						p.lexer.NextInsideJSXElement()
 					} else if p.lexer.Token == js_lexer.TLessThan {
 						// This may be removed in the future: https://github.com/facebook/jsx/issues/53
 						loc := p.lexer.Loc()
 						p.lexer.NextInsideJSXElement()
+						flags |= js_ast.PropertyWasShorthand
 						value = p.parseJSXElement(loc)
 
 						// The call to parseJSXElement() above doesn't consume the last
@@ -5199,7 +5204,11 @@ func (p *parser) parseJSXElement(loc logger.Loc) js_ast.Expr {
 	for {
 		switch p.lexer.Token {
 		case js_lexer.TStringLiteral:
-			nullableChildren = append(nullableChildren, js_ast.Expr{Loc: p.lexer.Loc(), Data: &js_ast.EString{Value: p.lexer.StringLiteral()}})
+			if p.options.jsx.Preserve {
+				nullableChildren = append(nullableChildren, js_ast.Expr{Loc: p.lexer.Loc(), Data: &js_ast.EJSXText{Raw: p.lexer.Raw()}})
+			} else {
+				nullableChildren = append(nullableChildren, js_ast.Expr{Loc: p.lexer.Loc(), Data: &js_ast.EString{Value: p.lexer.StringLiteral()}})
+			}
 			p.lexer.NextJSXElementChild()
 
 		case js_lexer.TOpenBrace:
@@ -5209,7 +5218,7 @@ func (p *parser) parseJSXElement(loc logger.Loc) js_ast.Expr {
 			// The expression is optional, and may be absent
 			if p.lexer.Token == js_lexer.TCloseBrace {
 				// Save comments even for absent expressions
-				nullableChildren = append(nullableChildren, js_ast.Expr{Loc: p.saveExprCommentsHere()})
+				nullableChildren = append(nullableChildren, js_ast.Expr{Loc: p.saveExprCommentsHere(), Data: nil})
 			} else {
 				if p.lexer.Token == js_lexer.TDotDotDot {
 					// TypeScript preserves "..." before JSX child expressions here.
@@ -12689,7 +12698,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 	// it doesn't affect these mitigations by ensuring that the mitigations are not
 	// applied in those cases (e.g. by adding an additional conditional check).
 	switch e := expr.Data.(type) {
-	case *js_ast.ENull, *js_ast.ESuper, *js_ast.EBoolean, *js_ast.EBigInt, *js_ast.EUndefined:
+	case *js_ast.ENull, *js_ast.ESuper, *js_ast.EBoolean, *js_ast.EBigInt, *js_ast.EUndefined, *js_ast.EJSXText:
 
 	case *js_ast.ENameOfSymbol:
 		e.Ref = p.symbolForMangledProp(p.loadNameFromRef(e.Ref))
@@ -13097,7 +13106,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 						propName := helpers.UTF16ToString(str.Value)
 						switch propName {
 						case "key":
-							if property.Flags.Has(js_ast.PropertyWasShorthand) {
+							if boolean, ok := property.ValueOrNil.Data.(*js_ast.EBoolean); ok && boolean.Value && property.Flags.Has(js_ast.PropertyWasShorthand) {
 								r := js_lexer.RangeOfIdentifier(p.source, property.Loc)
 								msg := logger.Msg{
 									Kind:  logger.Error,
diff --git a/internal/js_printer/js_printer.go b/internal/js_printer/js_printer.go
@@ -246,80 +246,6 @@ func (p *printer) printUnquotedUTF16(text []uint16, quote rune, flags printQuote
 	p.js = js
 }
 
-// Use JS strings for JSX attributes that need escape characters. Technically
-// the JSX specification doesn't say anything about using XML character escape
-// sequences, so JSX implementations may not be able to consume them. See
-// https://facebook.github.io/jsx/ for the specification.
-func (p *printer) canPrintTextAsJSXAttribute(text []uint16) (quote string, ok bool) {
-	single := true
-	double := true
-
-	for _, c := range text {
-		// Use JS strings for control characters
-		if c < firstASCII {
-			return "", false
-		}
-
-		// Use JS strings if we need to escape non-ASCII characters
-		if p.options.ASCIIOnly && c > lastASCII {
-			return "", false
-		}
-
-		switch c {
-		case '&':
-			// Use JS strings if the text would need to be escaped with "&amp;"
-			return "", false
-
-		case '"':
-			double = false
-			if !single {
-				break
-			}
-
-		case '\'':
-			single = false
-			if !double {
-				break
-			}
-		}
-	}
-
-	// Prefer duble quotes to single quotes
-	if double {
-		return "\"", true
-	}
-	if single {
-		return "'", true
-	}
-	return "", false
-}
-
-// Use JS strings for text inside JSX elements that need escape characters.
-// Technically the JSX specification doesn't say anything about using XML
-// character escape sequences, so JSX implementations may not be able to
-// consume them. See https://facebook.github.io/jsx/ for the specification.
-func (p *printer) canPrintTextAsJSXChild(text []uint16) bool {
-	for _, c := range text {
-		// Use JS strings for control characters
-		if c < firstASCII {
-			return false
-		}
-
-		// Use JS strings if we need to escape non-ASCII characters
-		if p.options.ASCIIOnly && c > lastASCII {
-			return false
-		}
-
-		switch c {
-		case '&', '<', '>', '{', '}':
-			// Use JS strings if the text would need to be escaped
-			return false
-		}
-	}
-
-	return true
-}
-
 // JSX tag syntax doesn't support character escapes so non-ASCII identifiers
 // must be printed as UTF-8 even when the charset is set to ASCII.
 func (p *printer) printJSXTag(tagOrNil js_ast.Expr) {
@@ -2143,26 +2069,28 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla
 
 			isMultiLine := p.willPrintExprCommentsAtLoc(property.ValueOrNil.Loc)
 
-			// Don't use shorthand syntax if it would discard comments
-			if !isMultiLine {
-				// Special-case string values
-				if str, ok := property.ValueOrNil.Data.(*js_ast.EString); ok {
-					if quote, ok := p.canPrintTextAsJSXAttribute(str.Value); ok {
-						p.print("=")
-						p.addSourceMapping(property.ValueOrNil.Loc)
-						p.print(quote)
-						p.print(helpers.UTF16ToString(str.Value))
-						p.print(quote)
-						continue
-					}
+			if property.Flags.Has(js_ast.PropertyWasShorthand) {
+				// Implicit "true" value
+				if boolean, ok := property.ValueOrNil.Data.(*js_ast.EBoolean); ok && boolean.Value {
+					continue
 				}
 
-				// Implicit "true" value
-				if boolean, ok := property.ValueOrNil.Data.(*js_ast.EBoolean); ok && boolean.Value && property.Flags.Has(js_ast.PropertyWasShorthand) {
+				// JSX element as JSX attribute value
+				if _, ok := property.ValueOrNil.Data.(*js_ast.EJSXElement); ok {
+					p.print("=")
+					p.printExpr(property.ValueOrNil, js_ast.LLowest, 0)
 					continue
 				}
 			}
 
+			// Special-case raw text
+			if text, ok := property.ValueOrNil.Data.(*js_ast.EJSXText); ok {
+				p.print("=")
+				p.addSourceMapping(property.ValueOrNil.Loc)
+				p.print(text.Raw)
+				continue
+			}
+
 			// Generic JS value
 			p.print("={")
 			if isMultiLine {
@@ -2197,30 +2125,13 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla
 		}
 		p.print(">")
 
-		isSingleLine := true
-		if !p.options.MinifyWhitespace {
-			isSingleLine = len(e.NullableChildren) < 2
-			if len(e.NullableChildren) == 1 {
-				if _, ok := e.NullableChildren[0].Data.(*js_ast.EJSXElement); !ok {
-					isSingleLine = true
-				}
-			}
-		}
-		if !isSingleLine {
-			p.options.Indent++
-		}
-
 		// Print the children
 		for _, childOrNil := range e.NullableChildren {
-			if !isSingleLine {
-				p.printNewline()
-				p.printIndent()
-			}
 			if _, ok := childOrNil.Data.(*js_ast.EJSXElement); ok {
 				p.printExpr(childOrNil, js_ast.LLowest, 0)
-			} else if str, ok := childOrNil.Data.(*js_ast.EString); ok && isSingleLine && p.canPrintTextAsJSXChild(str.Value) {
+			} else if text, ok := childOrNil.Data.(*js_ast.EJSXText); ok {
 				p.addSourceMapping(childOrNil.Loc)
-				p.print(helpers.UTF16ToString(str.Value))
+				p.print(text.Raw)
 			} else if childOrNil.Data != nil {
 				isMultiLine := p.willPrintExprCommentsAtLoc(childOrNil.Loc)
 				p.print("{")
@@ -2251,11 +2162,6 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla
 		}
 
 		// Print the closing tag
-		if !isSingleLine {
-			p.options.Indent--
-			p.printNewline()
-			p.printIndent()
-		}
 		p.addSourceMapping(e.CloseLoc)
 		p.print("</")
 		p.printJSXTag(e.TagOrNil)
diff --git a/internal/js_printer/js_printer_test.go b/internal/js_printer/js_printer_test.go
