Skip to content

Commit

Permalink
fix #2909: preserve comments for omitted AST nodes
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed Feb 13, 2023
1 parent 429d073 commit 631a563
Show file tree
Hide file tree
Showing 7 changed files with 171 additions and 38 deletions.
28 changes: 28 additions & 0 deletions internal/bundler_tests/bundler_default_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8054,6 +8054,20 @@ func TestCommentPreservationTransformJSX(t *testing.T) {
<div {/*before*/...x} />,
<div>{/*before*/x}</div>,
<>{/*before*/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</>,
)
`,
},
Expand All @@ -8076,6 +8090,20 @@ func TestCommentPreservationPreserveJSX(t *testing.T) {
<div {/*before*/...x} />,
<div>{/*before*/x}</div>,
<>{/*before*/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</>,
)
`,
},
Expand Down
54 changes: 52 additions & 2 deletions internal/bundler_tests/snapshots/snapshots_default.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,50 @@ console.log(
<>{
/*before*/
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"}
</>
);

================================================================================
Expand Down Expand Up @@ -746,7 +789,14 @@ console.log(
null,
/*before*/
x
)
),
// Comments on absent AST nodes
/* @__PURE__ */ React.createElement("div", null, "before", "after"),
/* @__PURE__ */ React.createElement("div", null, "before", "after"),
/* @__PURE__ */ React.createElement("div", null, "before", "after"),
/* @__PURE__ */ React.createElement(React.Fragment, null, "before", "after"),
/* @__PURE__ */ React.createElement(React.Fragment, null, "before", "after"),
/* @__PURE__ */ React.createElement(React.Fragment, null, "before", "after")
);

================================================================================
Expand Down
25 changes: 22 additions & 3 deletions internal/js_ast/js_ast.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -703,9 +703,28 @@ type EMangledProp struct {
}

type EJSXElement struct {
TagOrNil Expr
Properties []Property
Children []Expr
TagOrNil Expr
Properties []Property

// Note: This array may contain nil entries. Be careful about nil entries
// when iterating over this array.
//
// Each nil entry corresponds to the "JSXChildExpression_opt" part of the
// grammar (https://facebook.github.io/jsx/#prod-JSXChild):
//
// JSXChild :
// JSXText
// JSXElement
// JSXFragment
// { JSXChildExpression_opt }
//
// This is the "{}" part in "<a>{}</a>". We allow this because some people
// put comments there and then expect to be able to process them from
// esbuild's output. These absent AST nodes are completely omitted when
// JSX is transformed to JS. They are only present when JSX preservation is
// enabled.
NullableChildren []Expr

CloseLoc logger.Loc
IsTagSingleLine bool
}
Expand Down
60 changes: 39 additions & 21 deletions internal/js_parser/js_parser.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4970,19 +4970,22 @@ func (p *parser) parseJSXElement(loc logger.Loc) js_ast.Expr {
p.lexer.ExpectJSXElementChild(js_lexer.TGreaterThan)

// Parse the children of this element
children := []js_ast.Expr{}
nullableChildren := []js_ast.Expr{}
for {
switch p.lexer.Token {
case js_lexer.TStringLiteral:
children = append(children, js_ast.Expr{Loc: p.lexer.Loc(), Data: &js_ast.EString{Value: p.lexer.StringLiteral()}})
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:
// Use Next() instead of NextJSXElementChild() here since the next token is an expression
p.lexer.Next()

// The expression is optional, and may be absent
if p.lexer.Token != js_lexer.TCloseBrace {
if p.lexer.Token == js_lexer.TCloseBrace {
// Save comments even for absent expressions
nullableChildren = append(nullableChildren, js_ast.Expr{Loc: p.saveExprCommentsHere()})
} else {
if p.lexer.Token == js_lexer.TDotDotDot {
// TypeScript preserves "..." before JSX child expressions here.
// Babel gives the error "Spread children are not supported in React"
Expand All @@ -4992,9 +4995,9 @@ func (p *parser) parseJSXElement(loc logger.Loc) js_ast.Expr {
itemLoc := p.lexer.Loc()
p.markSyntaxFeature(compat.RestArgument, p.lexer.Range())
p.lexer.Next()
children = append(children, js_ast.Expr{Loc: itemLoc, Data: &js_ast.ESpread{Value: p.parseExpr(js_ast.LLowest)}})
nullableChildren = append(nullableChildren, js_ast.Expr{Loc: itemLoc, Data: &js_ast.ESpread{Value: p.parseExpr(js_ast.LLowest)}})
} else {
children = append(children, p.parseExpr(js_ast.LLowest))
nullableChildren = append(nullableChildren, p.parseExpr(js_ast.LLowest))
}
}

Expand All @@ -5007,7 +5010,7 @@ func (p *parser) parseJSXElement(loc logger.Loc) js_ast.Expr {

if p.lexer.Token != js_lexer.TSlash {
// This is a child element
children = append(children, p.parseJSXElement(lessThanLoc))
nullableChildren = append(nullableChildren, p.parseJSXElement(lessThanLoc))

// The call to parseJSXElement() above doesn't consume the last
// TGreaterThan because the caller knows what Next() function to call.
Expand All @@ -5034,11 +5037,11 @@ func (p *parser) parseJSXElement(loc logger.Loc) js_ast.Expr {
}

return js_ast.Expr{Loc: loc, Data: &js_ast.EJSXElement{
TagOrNil: startTagOrNil,
Properties: properties,
Children: children,
CloseLoc: lessThanLoc,
IsTagSingleLine: isSingleLine,
TagOrNil: startTagOrNil,
Properties: properties,
NullableChildren: nullableChildren,
CloseLoc: lessThanLoc,
IsTagSingleLine: isSingleLine,
}}

case js_lexer.TEndOfFile:
Expand Down Expand Up @@ -12232,9 +12235,11 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
}

// Visit children
if len(e.Children) > 0 {
for i, child := range e.Children {
e.Children[i] = p.visitExpr(child)
if len(e.NullableChildren) > 0 {
for i, childOrNil := range e.NullableChildren {
if childOrNil.Data != nil {
e.NullableChildren[i] = p.visitExpr(childOrNil)
}
}
}

Expand All @@ -12248,6 +12253,19 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
p.symbols[tag.Ref.InnerIndex].Flags |= js_ast.MustStartWithCapitalLetterForJSX
}
} else {
// Remove any nil children in the array (in place) before iterating over it
children := e.NullableChildren
{
end := 0
for _, childOrNil := range children {
if childOrNil.Data != nil {
children[end] = childOrNil
end++
}
}
children = children[:end]
}

// A missing tag is a fragment
if e.TagOrNil.Data == nil {
if p.options.jsx.AutomaticRuntime {
Expand Down Expand Up @@ -12288,8 +12306,8 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
} else {
args = append(args, js_ast.Expr{Loc: propsLoc, Data: js_ast.ENullShared})
}
if len(e.Children) > 0 {
args = append(args, e.Children...)
if len(children) > 0 {
args = append(args, children...)
}

// Call createElement()
Expand Down Expand Up @@ -12362,14 +12380,14 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
properties = append(properties, property)
}

isStaticChildren := len(e.Children) > 1
isStaticChildren := len(children) > 1

// Children are passed in as an explicit prop
if len(e.Children) > 0 {
childrenValue := e.Children[0]
if len(children) > 0 {
childrenValue := children[0]

if len(e.Children) > 1 {
childrenValue.Data = &js_ast.EArray{Items: e.Children}
if len(children) > 1 {
childrenValue.Data = &js_ast.EArray{Items: children}
} else if _, ok := childrenValue.Data.(*js_ast.ESpread); ok {
// TypeScript considers spread children to be static, but Babel considers
// it to be an error ("Spread children are not supported in React.").
Expand Down
6 changes: 6 additions & 0 deletions internal/js_parser/js_parser_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4901,6 +4901,12 @@ func TestJSX(t *testing.T) {
expectPrintedJSX(t, "<a>>></a>", "/* @__PURE__ */ React.createElement(\"a\", null, \">>\");\n")
expectPrintedJSX(t, "<a>{}</a>", "/* @__PURE__ */ React.createElement(\"a\", null);\n")
expectPrintedJSX(t, "<a>{/* comment */}</a>", "/* @__PURE__ */ React.createElement(\"a\", null);\n")
expectPrintedJSX(t, "<a>b{}</a>", "/* @__PURE__ */ React.createElement(\"a\", null, \"b\");\n")
expectPrintedJSX(t, "<a>b{/* comment */}</a>", "/* @__PURE__ */ React.createElement(\"a\", null, \"b\");\n")
expectPrintedJSX(t, "<a>{}c</a>", "/* @__PURE__ */ React.createElement(\"a\", null, \"c\");\n")
expectPrintedJSX(t, "<a>{/* comment */}c</a>", "/* @__PURE__ */ React.createElement(\"a\", null, \"c\");\n")
expectPrintedJSX(t, "<a>b{}c</a>", "/* @__PURE__ */ React.createElement(\"a\", null, \"b\", \"c\");\n")
expectPrintedJSX(t, "<a>b{/* comment */}c</a>", "/* @__PURE__ */ React.createElement(\"a\", null, \"b\", \"c\");\n")
expectPrintedJSX(t, "<a>{1, 2}</a>", "/* @__PURE__ */ React.createElement(\"a\", null, (1, 2));\n")
expectPrintedJSX(t, "<a>&lt;&gt;</a>", "/* @__PURE__ */ React.createElement(\"a\", null, \"<>\");\n")
expectPrintedJSX(t, "<a>&wrong;</a>", "/* @__PURE__ */ React.createElement(\"a\", null, \"&wrong;\");\n")
Expand Down
1 change: 1 addition & 0 deletions internal/js_parser/ts_parser_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2408,6 +2408,7 @@ func TestTSJSX(t *testing.T) {
expectParseErrorTSX(t, "const x = <number>1", "<stdin>: ERROR: Unexpected end of file before a closing \"number\" tag\n<stdin>: NOTE: The opening \"number\" tag is here:\n")

expectPrintedTSX(t, "<x>a{}c</x>", "/* @__PURE__ */ React.createElement(\"x\", null, \"a\", \"c\");\n")
expectPrintedTSX(t, "<x>a{/* comment */}c</x>", "/* @__PURE__ */ React.createElement(\"x\", null, \"a\", \"c\");\n")
expectPrintedTSX(t, "<x>a{b}c</x>", "/* @__PURE__ */ React.createElement(\"x\", null, \"a\", b, \"c\");\n")
expectPrintedTSX(t, "<x>a{...b}c</x>", "/* @__PURE__ */ React.createElement(\"x\", null, \"a\", ...b, \"c\");\n")

Expand Down
35 changes: 23 additions & 12 deletions internal/js_printer/js_printer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1922,7 +1922,7 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla
p.printIndent()
}
}
if e.TagOrNil.Data != nil && len(e.Children) == 0 {
if e.TagOrNil.Data != nil && len(e.NullableChildren) == 0 {
if e.IsTagSingleLine || len(e.Properties) == 0 {
p.printSpace()
}
Expand All @@ -1934,9 +1934,9 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla

isSingleLine := true
if !p.options.MinifyWhitespace {
isSingleLine = len(e.Children) < 2
if len(e.Children) == 1 {
if _, ok := e.Children[0].Data.(*js_ast.EJSXElement); !ok {
isSingleLine = len(e.NullableChildren) < 2
if len(e.NullableChildren) == 1 {
if _, ok := e.NullableChildren[0].Data.(*js_ast.EJSXElement); !ok {
isSingleLine = true
}
}
Expand All @@ -1946,31 +1946,42 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla
}

// Print the children
for _, child := range e.Children {
for _, childOrNil := range e.NullableChildren {
if !isSingleLine {
p.printNewline()
p.printIndent()
}
if _, ok := child.Data.(*js_ast.EJSXElement); ok {
p.printExpr(child, js_ast.LLowest, 0)
} else if str, ok := child.Data.(*js_ast.EString); ok && isSingleLine && p.canPrintTextAsJSXChild(str.Value) {
p.addSourceMapping(child.Loc)
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) {
p.addSourceMapping(childOrNil.Loc)
p.print(helpers.UTF16ToString(str.Value))
} else {
isMultiLine := p.willPrintExprCommentsAtLoc(child.Loc)
} else if childOrNil.Data != nil {
isMultiLine := p.willPrintExprCommentsAtLoc(childOrNil.Loc)
p.print("{")
if isMultiLine {
p.printNewline()
p.options.Indent++
p.printIndent()
}
p.printExpr(child, js_ast.LComma, 0)
p.printExpr(childOrNil, js_ast.LComma, 0)
if isMultiLine {
p.printNewline()
p.options.Indent--
p.printIndent()
}
p.print("}")
} else {
p.print("{")
if p.willPrintExprCommentsAtLoc(childOrNil.Loc) {
// Note: Some people use these comments for AST transformations
p.printNewline()
p.options.Indent++
p.printExprCommentsAfterCloseTokenAtLoc(childOrNil.Loc)
p.options.Indent--
p.printIndent()
}
p.print("}")
}
}

Expand Down

0 comments on commit 631a563

Please sign in to comment.