diff --git a/javascript/packages/core/src/ast-utils.ts b/javascript/packages/core/src/ast-utils.ts index 6c844a4cd..372f8e14d 100644 --- a/javascript/packages/core/src/ast-utils.ts +++ b/javascript/packages/core/src/ast-utils.ts @@ -19,11 +19,11 @@ import { } from "./nodes.js" import { - isNode, isAnyOf, isLiteralNode, isERBNode, isERBContentNode, + isHTMLCommentNode, areAllOfType, filterLiteralNodes } from "./node-type-guards.js" @@ -37,6 +37,12 @@ export type ERBOutputNode = ERBNode & { } } +export type ERBCommentNode = ERBNode & { + tag_opening: { + value: "<%#" + } +} + /** * Checks if a node is an ERB output node (generates content: <%= %> or <%== %>) */ @@ -47,6 +53,17 @@ export function isERBOutputNode(node: Node): node is ERBOutputNode { return ["<%=", "<%=="].includes(node.tag_opening?.value) } +/** + * Checks if a node is a ERB comment node (control flow: <%# %>) + */ +export function isERBCommentNode(node: Node): node is ERBCommentNode { + if (!isERBNode(node)) return false + if (!node.tag_opening?.value) return false + + return node.tag_opening?.value === "<%#" || (node.tag_opening?.value !== "<%#" && (node.content?.value || "").trimStart().startsWith("#")) +} + + /** * Checks if a node is a non-output ERB node (control flow: <% %>) */ @@ -200,8 +217,8 @@ export function getTagName(node: HTMLElementNode | HTMLOpenTagNode | HTMLCloseTa /** * Check if a node is a comment (HTML comment or ERB comment) */ -export function isCommentNode(node: Node): boolean { - return isNode(node, HTMLCommentNode) || (isERBNode(node) && !isERBControlFlowNode(node)) +export function isCommentNode(node: Node): node is HTMLCommentNode | ERBCommentNode { + return isHTMLCommentNode(node) || isERBCommentNode(node) } /** diff --git a/javascript/packages/formatter/src/format-helpers.ts b/javascript/packages/formatter/src/format-helpers.ts index 56abd291e..7fa1203ba 100644 --- a/javascript/packages/formatter/src/format-helpers.ts +++ b/javascript/packages/formatter/src/format-helpers.ts @@ -56,17 +56,6 @@ export const SPACEABLE_CONTAINERS = new Set([ 'figure', 'details', 'summary', 'dialog', 'fieldset' ]) -export const TIGHT_GROUP_PARENTS = new Set([ - 'ul', 'ol', 'nav', 'select', 'datalist', 'optgroup', 'tr', 'thead', - 'tbody', 'tfoot' -]) - -export const TIGHT_GROUP_CHILDREN = new Set([ - 'li', 'option', 'td', 'th', 'dt', 'dd' -]) - -export const SPACING_THRESHOLD = 3 - /** * Token list attributes that contain space-separated values and benefit from * spacing around ERB content for readability diff --git a/javascript/packages/formatter/src/format-printer.ts b/javascript/packages/formatter/src/format-printer.ts index 6dd97ec57..bf6c0f1cc 100644 --- a/javascript/packages/formatter/src/format-printer.ts +++ b/javascript/packages/formatter/src/format-printer.ts @@ -13,6 +13,8 @@ import { isERBNode, isCommentNode, isERBControlFlowNode, + isERBCommentNode, + isERBOutputNode, filterNodes, } from "@herb-tools/core" @@ -47,9 +49,6 @@ import { FORMATTABLE_ATTRIBUTES, INLINE_ELEMENTS, SPACEABLE_CONTAINERS, - SPACING_THRESHOLD, - TIGHT_GROUP_CHILDREN, - TIGHT_GROUP_PARENTS, TOKEN_LIST_ATTRIBUTES, } from "./format-helpers.js" @@ -288,6 +287,86 @@ export class FormatPrinter extends Printer { return nodes.filter(child => isNoneOf(child, HTMLAttributeNode, WhitespaceNode)) } + + /** + * Determine if an HTML element or ERB block would format to multiple lines + * Used for spacing multiline elements + * + * We render the node and check if the output contains newlines. + * This is the most accurate and reliable way to determine multiline status. + */ + private isMultilineElement(node: Node): boolean { + try { + return this.capture(() => this.visit(node)).join("\n").trim().includes("\n") + } catch (e) { + return false + } + } + + /** + * Get a grouping key for a node (tag name for HTML, ERB type for ERB) + */ + private getGroupingKey(node: Node): string | null { + if (isNode(node, HTMLElementNode)) { + return getTagName(node) + } + + if (isERBOutputNode(node)) return "erb-output" + if (isERBCommentNode(node)) return "erb-comment" + if (isERBNode(node)) return "erb-code" + + return null + } + + /** + * Detect groups of consecutive same-tag/same-type single-line elements + * Returns a map of index -> group info for efficient lookup + */ + private detectTagGroups(siblings: Node[]): Map { + const groupMap = new Map() + const meaningfulNodes: Array<{ index: number; groupKey: string }> = [] + + for (let i = 0; i < siblings.length; i++) { + const node = siblings[i] + + if (!this.isMultilineElement(node)) { + const groupKey = this.getGroupingKey(node) + + if (groupKey) { + meaningfulNodes.push({ index: i, groupKey }) + } + } + } + + let groupStart = 0 + + while (groupStart < meaningfulNodes.length) { + const startGroupKey = meaningfulNodes[groupStart].groupKey + let groupEnd = groupStart + + while (groupEnd + 1 < meaningfulNodes.length && meaningfulNodes[groupEnd + 1].groupKey === startGroupKey) { + groupEnd++ + } + + if (groupEnd > groupStart) { + const groupStartIndex = meaningfulNodes[groupStart].index + const groupEndIndex = meaningfulNodes[groupEnd].index + + for (let i = groupStart; i <= groupEnd; i++) { + groupMap.set(meaningfulNodes[i].index, { + tagName: startGroupKey, + groupStart: groupStartIndex, + groupEnd: groupEndIndex + }) + } + } + + groupStart = groupEnd + 1 + } + + return groupMap + } + /** * Determine if spacing should be added between sibling elements * @@ -303,69 +382,68 @@ export class FormatPrinter extends Printer { * @param hasExistingSpacing - Whether user-added spacing already exists * @returns true if spacing should be added before the current element */ - private shouldAddSpacingBetweenSiblings( - parentElement: HTMLElementNode | null, - siblings: Node[], - currentIndex: number, - hasExistingSpacing: boolean - ): boolean { - if (hasExistingSpacing) { + private shouldAddSpacingBetweenSiblings(parentElement: HTMLElementNode | null, siblings: Node[], currentIndex: number, hasExistingSpacing: boolean): boolean { + if (hasExistingSpacing) return true + + const currentNode = siblings[currentIndex] + const previousMeaningfulIndex = findPreviousMeaningfulSibling(siblings, currentIndex) + const previousNode = previousMeaningfulIndex !== -1 ? siblings[previousMeaningfulIndex] : null + + if (previousNode && (isNode(previousNode, XMLDeclarationNode) || isNode(previousNode, HTMLDoctypeNode))) { return true } const hasMixedContent = siblings.some(child => isNode(child, HTMLTextNode) && child.content.trim() !== "") - if (hasMixedContent) { - return false - } + if (hasMixedContent) return false - const meaningfulSiblings = siblings.filter(child => isNonWhitespaceNode(child)) + const isCurrentComment = isCommentNode(currentNode) + const isPreviousComment = previousNode ? isCommentNode(previousNode) : false + const isCurrentMultiline = this.isMultilineElement(currentNode) + const isPreviousMultiline = previousNode ? this.isMultilineElement(previousNode) : false - if (meaningfulSiblings.length < SPACING_THRESHOLD) { - return false + if (isPreviousComment && !isCurrentComment && (isNode(currentNode, HTMLElementNode) || isERBNode(currentNode))) { + return isPreviousMultiline && isCurrentMultiline } - const parentTagName = parentElement ? getTagName(parentElement) : null - - if (parentTagName && TIGHT_GROUP_PARENTS.has(parentTagName)) { + if (isPreviousComment && isCurrentComment) { return false } + if (isCurrentMultiline || isPreviousMultiline) { + return true + } + + const meaningfulSiblings = siblings.filter(child => isNonWhitespaceNode(child)) + const parentTagName = parentElement ? getTagName(parentElement) : null const isSpaceableContainer = !parentTagName || (parentTagName && SPACEABLE_CONTAINERS.has(parentTagName)) if (!isSpaceableContainer && meaningfulSiblings.length < 5) { return false } - const currentNode = siblings[currentIndex] - const previousMeaningfulIndex = findPreviousMeaningfulSibling(siblings, currentIndex) - const isCurrentComment = isCommentNode(currentNode) + const tagGroups = this.detectTagGroups(siblings) + const currentGroup = tagGroups.get(currentIndex) + const previousGroup = previousNode ? tagGroups.get(previousMeaningfulIndex) : undefined - if (previousMeaningfulIndex !== -1) { - const previousNode = siblings[previousMeaningfulIndex] - const isPreviousComment = isCommentNode(previousNode) + if (currentGroup && previousGroup && currentGroup.groupStart === previousGroup.groupStart && currentGroup.groupEnd === previousGroup.groupEnd) { + return false + } - if (isPreviousComment && !isCurrentComment && (isNode(currentNode, HTMLElementNode) || isERBNode(currentNode))) { - return false - } + if (previousGroup && previousGroup.groupEnd === previousMeaningfulIndex) { + return true + } - if (isPreviousComment && isCurrentComment) { - return false - } + const allSingleLineHTMLElements = meaningfulSiblings.every(node => isNode(node, HTMLElementNode) && !this.isMultilineElement(node)) + + if (allSingleLineHTMLElements && tagGroups.size === 0) { + return false } if (isNode(currentNode, HTMLElementNode)) { const currentTagName = getTagName(currentNode) - if (INLINE_ELEMENTS.has(currentTagName)) { - return false - } - - if (TIGHT_GROUP_CHILDREN.has(currentTagName)) { - return false - } - - if (currentTagName === 'a' && parentTagName === 'nav') { + if (currentTagName && INLINE_ELEMENTS.has(currentTagName)) { return false } } @@ -676,7 +754,21 @@ export class FormatPrinter extends Printer { } if (isNonWhitespaceNode(child) && lastWasMeaningful && !hasHandledSpacing) { - this.push("") + const previousNode = i > 0 ? children[i - 1] : null + const hasExistingSpacing = !!(previousNode && isNode(previousNode, HTMLTextNode) && + previousNode.content.trim() === "" && + (previousNode.content.includes('\n\n') || previousNode.content.split('\n').length > 2)) + + const shouldAddSpacing = this.shouldAddSpacingBetweenSiblings( + null, + children, + i, + hasExistingSpacing + ) + + if (shouldAddSpacing) { + this.push("") + } } this.visit(child) @@ -1189,8 +1281,7 @@ export class FormatPrinter extends Printer { } visitERBContentNode(node: ERBContentNode) { - // TODO: this feels hacky - if (node.tag_opening?.value === "<%#") { + if (isERBCommentNode(node)) { this.visitERBCommentNode(node) } else { this.printERBNode(node) diff --git a/javascript/packages/formatter/test/cli.test.ts b/javascript/packages/formatter/test/cli.test.ts index 9a3c72405..8c7622fd2 100644 --- a/javascript/packages/formatter/test/cli.test.ts +++ b/javascript/packages/formatter/test/cli.test.ts @@ -94,6 +94,7 @@ describe("CLI Binary", () => { expect(formattedContent).toBe(dedent`
<%= "Hello" %> +

World

` + '\n') diff --git a/javascript/packages/formatter/test/document-formatting.test.ts b/javascript/packages/formatter/test/document-formatting.test.ts index 1f7d4c633..4c1299cda 100644 --- a/javascript/packages/formatter/test/document-formatting.test.ts +++ b/javascript/packages/formatter/test/document-formatting.test.ts @@ -147,7 +147,6 @@ describe("Document-level formatting", () => { const result = formatter.format(source) expect(result).toEqual(dedent` <% page_title = "User Profile" %> - <% user_data = { name: "John", age: 30 } %> @@ -236,9 +235,7 @@ describe("Document-level formatting", () => { const result = formatter.format(source) expect(result).toEqual(dedent`

Title

-

Content

-
Footer
`) }) diff --git a/javascript/packages/formatter/test/erb-formatter/erb-formatter-additional.test.ts b/javascript/packages/formatter/test/erb-formatter/erb-formatter-additional.test.ts index d9f14add6..8bc865623 100644 --- a/javascript/packages/formatter/test/erb-formatter/erb-formatter-additional.test.ts +++ b/javascript/packages/formatter/test/erb-formatter/erb-formatter-additional.test.ts @@ -85,7 +85,6 @@ describe("ERB Formatter Additional Tests", () => { expect(result).toBe(dedent` <%- vite_client_tag %> - <%= vite_typescript_tag "application", "data-turbo-track": "reload", defer: true %> <%= stylesheet_link_tag "tailwind", @@ -159,6 +158,7 @@ describe("ERB Formatter Additional Tests", () => {
Short text +

In the event we decide to issue a refund, we will reimburse you no later than fourteen (14) days from the date on which we make that diff --git a/javascript/packages/formatter/test/erb-formatter/erb-formatter-compat.test.ts b/javascript/packages/formatter/test/erb-formatter/erb-formatter-compat.test.ts index d71587e39..8582ceb01 100644 --- a/javascript/packages/formatter/test/erb-formatter/erb-formatter-compat.test.ts +++ b/javascript/packages/formatter/test/erb-formatter/erb-formatter-compat.test.ts @@ -120,7 +120,6 @@ describe("ERB Formatter Compatibility Tests", () => { expect(result).toEqual(dedent` <% if eeee then "b" else c end %> - <% if eeee then a else c end %> `) }) @@ -147,7 +146,6 @@ describe("ERB Formatter Compatibility Tests", () => { expect(result).toEqual(dedent`

>
-
>
`) }) @@ -328,7 +326,6 @@ describe("ERB Formatter Compatibility Tests", () => { expect(result).toEqual(dedent` <%# This is a comment %> -
Content
<% # Another comment style %> @@ -371,6 +368,7 @@ describe("ERB Formatter Compatibility Tests", () => { <% categories.each do |category| %>

<%= category.name %>

+ <% category.items.each do |item| %>
<%= item.name %>
<% end %> diff --git a/javascript/packages/formatter/test/erb-formatter/erb-formatter-fixtures.test.ts b/javascript/packages/formatter/test/erb-formatter/erb-formatter-fixtures.test.ts index 0c4ab6a8a..0095c9d8b 100644 --- a/javascript/packages/formatter/test/erb-formatter/erb-formatter-fixtures.test.ts +++ b/javascript/packages/formatter/test/erb-formatter/erb-formatter-fixtures.test.ts @@ -167,6 +167,7 @@ describe("ERB Formatter Fixture Tests", () => { <% when 'admin', 'moderator' %>

Admin Controls

+ <% if user.permissions.include?('delete') %> <% end %> @@ -314,13 +315,11 @@ describe("ERB Formatter Fixture Tests", () => { expect(result).toBe(dedent` <% if eeee then "b" else c end %> - <% if eeee then a else c end %> <% if longlonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglong then a else c end %>
>
-
>
{ expect(result).toBe(dedent`

Welcome

+ <% if user.present? %>

Hello <%= user.name %>!

<% else %> @@ -409,6 +409,7 @@ describe("ERB Formatter Fixture Tests", () => { <%= page_title %> + <% content_for :navigation do %>