Skip to content

Commit 2f2ba1c

Browse files
marcorothmarcoroth
authored
Formatter: Preserve user newlines for block elements with ERB children (#1295)
Previously, the formatter would inline ERB children in block elements when the content fit within `maxLineLength`, even when the user intentionally placed them on separate lines. This pull request extends the existing mixed-content newline preservation (#1279) to also cover block elements with ERB children. Resolves #1181
1 parent 1615346 commit 2f2ba1c

File tree

6 files changed

+211
-76
lines changed

6 files changed

+211
-76
lines changed

‎javascript/packages/formatter/src/format-printer.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1288,14 +1288,18 @@ export class FormatPrinter extends Printer implements TextFlowDelegate, Attribut
12881288
if (allChildrenAreERB) return false
12891289
}
12901290

1291-
if (!isInlineElement(tagName) && hasMixedTextAndInlineContent(children) && openTagClosing) {
1291+
if (!isInlineElement(tagName) && openTagClosing) {
12921292
const first = children[0]
12931293
const startsOnNewLine = first.location.start.line > openTagClosing.location.end.line
12941294
const hasLeadingNewline = isNode(first, HTMLTextNode) && /^\s*\n/.test(first.content)
12951295
const contentStartsOnNewLine = startsOnNewLine || hasLeadingNewline
12961296

12971297
if (contentStartsOnNewLine) {
1298-
return false
1298+
const hasERBChildren = children.some(child => isERBNode(child))
1299+
1300+
if (hasERBChildren || hasMixedTextAndInlineContent(children)) {
1301+
return false
1302+
}
12991303
}
13001304
}
13011305

‎javascript/packages/formatter/test/erb-formatter/erb-formatter-compat.test.ts‎

Lines changed: 15 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
import { describe, test, expect, beforeAll } from "vitest"
22
import { Herb } from "@herb-tools/node-wasm"
33
import { Formatter } from "../../src"
4+
import { createExpectFormattedToMatch } from "../helpers"
45
import dedent from "dedent"
56

67
let formatter: Formatter
8+
let expectFormattedTotMatch: ReturnType<typeof createExpectFormattedToMatch>
79

810
describe("ERB Formatter Compatibility Tests", () => {
911
beforeAll(async () => {
@@ -13,6 +15,8 @@ describe("ERB Formatter Compatibility Tests", () => {
1315
indentWidth: 2,
1416
maxLineLength: 80,
1517
})
18+
19+
expectFormattedTotMatch = createExpectFormattedToMatch(formatter)
1620
})
1721

1822
describe("Attributes handling", () => {
@@ -111,40 +115,20 @@ describe("ERB Formatter Compatibility Tests", () => {
111115

112116
describe("ERB control structures", () => {
113117
test("formats simple if-then-else expressions", () => {
114-
const source = dedent`
115-
<% if eeee then "b" else c end %>
116-
<% if eeee then a else c end %>
117-
`
118-
119-
const result = formatter.format(source)
120-
121-
expect(result).toEqual(dedent`
118+
expectFormattedTotMatch(dedent`
122119
<% if eeee then "b" else c end %>
123120
<% if eeee then a else c end %>
124121
`)
125122
})
126123

127124
test("formats long if-then-else to multiline", () => {
128-
const source = dedent`
129-
<% if longlonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglong then a else c end %>
130-
`
131-
132-
const result = formatter.format(source)
133-
134-
expect(result).toEqual(dedent`
125+
expectFormattedTotMatch(dedent`
135126
<% if longlonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglong then a else c end %>
136127
`)
137128
})
138129

139130
test("handles ERB in tag attributes", () => {
140-
const source = dedent`
141-
<div <% if eeee then "b" else c end %>></div>
142-
<div <% if eeee then a else c end %>></div>
143-
`
144-
145-
const result = formatter.format(source)
146-
147-
expect(result).toEqual(dedent`
131+
expectFormattedTotMatch(dedent`
148132
<div <% if eeee then "b" else c end %>></div>
149133
<div <% if eeee then a else c end %>></div>
150134
`)
@@ -243,62 +227,38 @@ describe("ERB Formatter Compatibility Tests", () => {
243227

244228
describe("UTF-8 handling", () => {
245229
test("properly handles UTF-8 characters", () => {
246-
const source = dedent`
247-
<div>🍰 UTF-8 content with émojis and special characters ñ</div>
248-
`
249-
250-
const result = formatter.format(source)
251-
252-
expect(result).toEqual(dedent`
230+
expectFormattedTotMatch(dedent`
253231
<div>🍰 UTF-8 content with émojis and special characters ñ</div>
254232
`)
255233
})
256234
})
257235

258236
describe("Yield statements", () => {
259237
test("formats yield statements correctly", () => {
260-
const source = dedent`
238+
expectFormattedTotMatch(dedent`
261239
<div>
262240
<%= yield %>
263241
</div>
264-
`
265-
266-
const result = formatter.format(source)
242+
`)
267243

268-
expect(result).toEqual(dedent`
244+
expectFormattedTotMatch(dedent`
269245
<div><%= yield %></div>
270246
`)
271247
})
272248

273249
test("formats yield with arguments", () => {
274-
const source = dedent`
250+
expectFormattedTotMatch(dedent`
275251
<div>
276252
<%= yield(:header) %>
277253
<%= yield :footer, class: "mt-4" %>
278254
</div>
279-
`
280-
281-
const result = formatter.format(source)
282-
expect(result).toEqual(source)
255+
`)
283256
})
284257
})
285258

286259
describe("Case statements", () => {
287260
test("formats case/when statements", () => {
288-
const source = dedent`
289-
<% case status
290-
when 'active' %>
291-
<span class="badge-active">Active</span>
292-
<% when 'inactive' %>
293-
<span class="badge-inactive">Inactive</span>
294-
<% else %>
295-
<span class="badge-unknown">Unknown</span>
296-
<% end %>
297-
`
298-
299-
const result = formatter.format(source)
300-
301-
expect(result).toBe(dedent`
261+
expectFormattedTotMatch(dedent`
302262
<% case status
303263
when 'active' %>
304264
<span class="badge-active">Active</span>
@@ -332,15 +292,7 @@ describe("ERB Formatter Compatibility Tests", () => {
332292

333293
describe("Block statements", () => {
334294
test("formats blocks correctly", () => {
335-
const source = dedent`
336-
<% items.each do |item| %>
337-
<div><%= item.name %></div>
338-
<% end %>
339-
`
340-
341-
const result = formatter.format(source)
342-
343-
expect(result).toEqual(dedent`
295+
expectFormattedTotMatch(dedent`
344296
<% items.each do |item| %>
345297
<div><%= item.name %></div>
346298
<% end %>

‎javascript/packages/formatter/test/erb-formatter/erb-formatter-fixtures.test.ts‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,9 @@ describe("ERB Formatter Fixture Tests", () => {
421421
</nav>
422422
<% end %>
423423
424-
<main><%= yield %></main>
424+
<main>
425+
<%= yield %>
426+
</main>
425427
</body>
426428
</html>
427429
`)
@@ -462,7 +464,9 @@ describe("ERB Formatter Fixture Tests", () => {
462464
463465
<% link_to "string", path, opt: "212", options: "222sdasdasd" %>
464466
465-
<div><%= react_component({ greeting: 'react-rails.' }) %></div>
467+
<div>
468+
<%= react_component({ greeting: 'react-rails.' }) %>
469+
</div>
466470
</div>
467471
</div>
468472
</div>
@@ -563,13 +567,21 @@ describe("ERB Formatter Fixture Tests", () => {
563567

564568
expect(result).toBe(dedent`
565569
<div class="layout">
566-
<header><%= yield :header %></header>
570+
<header>
571+
<%= yield :header %>
572+
</header>
567573
568-
<main><%= yield %></main>
574+
<main>
575+
<%= yield %>
576+
</main>
569577
570-
<aside><%= yield :sidebar if content_for?(:sidebar) %></aside>
578+
<aside>
579+
<%= yield :sidebar if content_for?(:sidebar) %>
580+
</aside>
571581
572-
<footer><%= yield :footer %></footer>
582+
<footer>
583+
<%= yield :footer %>
584+
</footer>
573585
</div>
574586
`)
575587
})

‎javascript/packages/formatter/test/erb/erb.test.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,9 @@ describe("@herb-tools/formatter", () => {
9898
const result = formatter.format(source)
9999
expect(result).toEqual(dedent`
100100
<div>
101-
<h1 class="<%= classes %>"><%= title %></h1>
101+
<h1 class="<%= classes %>">
102+
<%= title %>
103+
</h1>
102104
</div>
103105
`)
104106
})
@@ -619,7 +621,9 @@ describe("@herb-tools/formatter", () => {
619621
620622
<body style="font-family: 'DM Sans', Arial, sans-serif;">
621623
<% if content_for?(:preheader) %>
622-
<div class="hidden"><%= yield :preheader %></div>
624+
<div class="hidden">
625+
<%= yield :preheader %>
626+
</div>
623627
<% end %>
624628
625629
<div

‎javascript/packages/formatter/test/erb/unless.test.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,9 @@ describe("@herb-tools/formatter", () => {
155155
<% [1, 2].each do %>
156156
<% return unless @content.present? %>
157157
158-
<div class="content"><%= @content %></div>
158+
<div class="content">
159+
<%= @content %>
160+
</div>
159161
<% end %>
160162
`
161163

@@ -234,9 +236,13 @@ describe("@herb-tools/formatter", () => {
234236
<% next unless item.visible? %>
235237
236238
<% unless item.featured? %>
237-
<div class="regular"><%= item.name %></div>
239+
<div class="regular">
240+
<%= item.name %>
241+
</div>
238242
<% else %>
239-
<div class="featured"><%= item.name %></div>
243+
<div class="featured">
244+
<%= item.name %>
245+
</div>
240246
<% end %>
241247
<% end %>
242248
`

0 commit comments

Comments
 (0)