diff --git a/javascript/packages/analysis/src/partial-index-builder.ts b/javascript/packages/analysis/src/partial-index-builder.ts index 0039fb330..661217a35 100644 --- a/javascript/packages/analysis/src/partial-index-builder.ts +++ b/javascript/packages/analysis/src/partial-index-builder.ts @@ -45,11 +45,14 @@ export async function buildPartialIndex(herb: HerbBackend, projectPath: string): const viewRoot = await findViewRoot(projectPath) const files = await partialsIn(projectPath, viewRoot) const declarations = new Map() + const filesByName = new Map() for (const file of files.sort()) { const name = partialNameForFile(file, viewRoot) if (name === null) continue + filesByName.set(name, [...(filesByName.get(name) ?? []), file]) + const existing = declarations.get(name) if (existing && !outranksTemplate(file, existing.file)) continue @@ -58,7 +61,7 @@ export async function buildPartialIndex(herb: HerbBackend, projectPath: string): if (declaration) declarations.set(name, declaration) } - return new PartialIndex(viewRoot, declarations) + return new PartialIndex([viewRoot], declarations, filesByName) } export function partialIndexFrom(data: SerializedPartialIndex | undefined): PartialIndex | undefined { diff --git a/javascript/packages/analysis/src/partial-index.ts b/javascript/packages/analysis/src/partial-index.ts index b06e7d961..14d6bb9a6 100644 --- a/javascript/packages/analysis/src/partial-index.ts +++ b/javascript/packages/analysis/src/partial-index.ts @@ -1,5 +1,5 @@ import { isERBStrictLocalsNode, isRubyParameterNode } from "@herb-tools/core" -import { PARTIAL_EXTENSIONS, partialNameForFile, resolvePartial } from "./partial-resolution" +import { PARTIAL_EXTENSIONS, partialNameForRoots, resolvePartial } from "./partial-resolution" import type { DocumentNode } from "@herb-tools/core" import type { PartialPaths } from "./partial-resolution" @@ -23,7 +23,7 @@ export interface PartialDeclaration { } export interface SerializedPartialIndex { - viewRoot: string + viewRoots: string[] partials: Record } @@ -90,24 +90,24 @@ export function declarationFromDocument(document: DocumentNode, file: string): P } export class PartialIndex { - readonly viewRoot: string + readonly viewRoots: string[] private readonly declarations: Map private readonly files: PartialPaths private readonly byFile: Map static from(data: SerializedPartialIndex): PartialIndex { - return new PartialIndex(data.viewRoot, new Map(Object.entries(data.partials))) + return new PartialIndex(data.viewRoots, new Map(Object.entries(data.partials))) } - constructor(viewRoot: string, declarations: Map) { - this.viewRoot = viewRoot + constructor(viewRoots: string[], declarations: Map, filesByName?: Map) { + this.viewRoots = viewRoots this.declarations = declarations this.files = new Map() this.byFile = new Map() for (const [name, declaration] of declarations) { - this.files.set(name, declaration.file) + this.files.set(name, filesByName?.get(name) ?? declaration.file) this.byFile.set(declaration.file, declaration) } } @@ -117,7 +117,7 @@ export class PartialIndex { } lookup(partialName: string, sourceFile: string | undefined): PartialDeclaration | null { - const file = resolvePartial(partialName, sourceFile ?? "", this.files, this.viewRoot) + const file = resolvePartial(partialName, sourceFile ?? "", this.files, this.viewRoots) if (file === null) return null @@ -125,7 +125,7 @@ export class PartialIndex { } update(declaration: PartialDeclaration): string | null { - const name = partialNameForFile(declaration.file, this.viewRoot) + const name = partialNameForRoots(declaration.file, this.viewRoots) if (name === null) return null const existing = this.declarations.get(name) @@ -144,7 +144,7 @@ export class PartialIndex { } remove(file: string): string | null { - const name = partialNameForFile(file, this.viewRoot) + const name = partialNameForRoots(file, this.viewRoots) if (name === null) return null const existing = this.declarations.get(name) @@ -162,6 +162,6 @@ export class PartialIndex { } toJSON(): SerializedPartialIndex { - return { viewRoot: this.viewRoot, partials: Object.fromEntries(this.declarations) } + return { viewRoots: this.viewRoots, partials: Object.fromEntries(this.declarations) } } } diff --git a/javascript/packages/analysis/src/partial-resolution.ts b/javascript/packages/analysis/src/partial-resolution.ts index d20a08ad1..1e05eafbf 100644 --- a/javascript/packages/analysis/src/partial-resolution.ts +++ b/javascript/packages/analysis/src/partial-resolution.ts @@ -15,7 +15,7 @@ export const PARTIAL_GLOB_PATTERN = `_${TEMPLATE_GLOB_PATTERN}` const PARTIAL_PREFIX = "_" const APPLICATION_DIRECTORY = "application" -export type PartialPaths = Map +export type PartialPaths = Map function normalize(path: string): string { const separated = path.replace(/\\/g, "/") @@ -50,12 +50,120 @@ function relativeToViewRoot(path: string, viewRoot: string): string | null { return normalizedPath.slice(normalizedRoot.length + 1) } +export function relativeToViewRoots(path: string, viewRoots: string[]): [number, string] | null { + for (const [index, root] of viewRoots.entries()) { + const relative = relativeToViewRoot(path, root) + + if (relative !== null) return [index, relative] + } + + return null +} + +export function partialNameForRoots(filePath: string, viewRoots: string[]): string | null { + for (const root of viewRoots) { + const name = partialNameForFile(filePath, root) + + if (name !== null) return name + } + + return null +} + +export function templateNameForRoots(filePath: string, viewRoots: string[]): string | null { + for (const root of viewRoots) { + const name = templateNameForFile(filePath, root) + + if (name !== null) return name + } + + return null +} + +export function rootIndexFor(filePath: string, viewRoots: string[]): number { + return relativeToViewRoots(filePath, viewRoots)?.[0] ?? viewRoots.length +} + export function projectRelativePath(filePath: string, projectPath: string | undefined): string { if (!projectPath) return normalize(filePath) return relativeToViewRoot(filePath, projectPath) ?? normalize(filePath) } +export function formatOf(filePath: string): string | null { + const name = basename(normalize(filePath)) + const dot = name.indexOf(".") + + if (dot === -1) return null + + const extension = name.slice(dot) + const stripped = extension.endsWith(".erb") + ? extension.slice(0, -".erb".length) + : extension.endsWith(".herb") + ? extension.slice(0, -".herb".length) + : null + + if (stripped === null) return null + + const segments = stripped.startsWith(".") ? stripped.slice(1) : stripped + const last = segments.split(".").pop() ?? segments + const format = last.split("+")[0] ?? last + + return format === "" ? null : format +} + +export function hasLocale(filePath: string): boolean { + const name = basename(normalize(filePath)) + const dot = name.indexOf(".") + + if (dot === -1) return false + + const extension = name.slice(dot) + const stripped = extension.endsWith(".erb") + ? extension.slice(0, -".erb".length) + : extension.endsWith(".herb") + ? extension.slice(0, -".herb".length) + : null + + if (stripped === null) return false + + const segments = (stripped.startsWith(".") ? stripped.slice(1) : stripped).split(".") + + return segments.length > 1 +} + +export function variantOf(filePath: string): string | null { + const name = basename(normalize(filePath)) + const dot = name.indexOf(".") + + if (dot === -1) return null + + const extension = name.slice(dot) + const stripped = extension.endsWith(".erb") + ? extension.slice(0, -".erb".length) + : extension.endsWith(".herb") + ? extension.slice(0, -".herb".length) + : null + + if (stripped === null) return null + + const plus = stripped.indexOf("+") + + if (plus === -1) return null + + const variant = stripped.slice(plus + 1) + + return variant === "" ? null : variant +} + +export function withoutTemplateExtension(partialName: string): string { + for (const extension of PARTIAL_EXTENSIONS) { + if (partialName.endsWith(extension)) return partialName.slice(0, -extension.length) + } + + return partialName +} + export function isTemplatePath(filePath: string): boolean { const name = basename(normalize(filePath)) @@ -111,6 +219,16 @@ export function templateNameForFile(filePath: string, viewRoot: string): string return directory === "." ? withoutExtension : `${directory}/${withoutExtension}` } +export function layoutCandidatesForRoots(templateFile: string, viewRoots: string[]): string[] { + for (const root of viewRoots) { + const candidates = layoutCandidatesFor(templateFile, root) + + if (candidates.length > 0) return candidates + } + + return [] +} + export function layoutCandidatesFor(templateFile: string, viewRoot: string): string[] { const relative = relativeToViewRoot(normalize(templateFile), viewRoot) @@ -136,23 +254,50 @@ export function layoutCandidatesFor(templateFile: string, viewRoot: string): str return candidates } -export function resolvePartial(partialName: string, sourceFile: string, index: PartialPaths, viewRoot: string): string | null { +function pickForCaller(candidates: string | string[], sourceFile: string): string | null { + if (!Array.isArray(candidates)) return candidates + if (candidates.length === 0) return null + + const format = formatOf(sourceFile) + + if (format === null) return candidates[0] ?? null + + const ranked = [...candidates].sort((a, b) => rankForFormat(a, format) - rankForFormat(b, format)) + + return ranked[0] ?? null +} + +function rankForFormat(file: string, format: string): number { + const candidate = formatOf(file) + const matches = candidate === format ? 0 : candidate === null ? 1 : 2 + + return matches * 4 + (variantOf(file) === null ? 0 : 2) + (hasLocale(file) ? 1 : 0) +} + +export function resolvePartial( + partialName: string, + sourceFile: string, + index: PartialPaths, + viewRoots: string[] +): string | null { + partialName = withoutTemplateExtension(partialName) + const exact = index.get(partialName) - if (exact !== undefined) return exact + if (exact !== undefined) return pickForCaller(exact, sourceFile) - const sourceDirectory = relativeToViewRoot(dirname(normalize(sourceFile)), viewRoot) + const sourceDirectory = relativeToViewRoots(dirname(normalize(sourceFile)), viewRoots)?.[1] ?? null if (sourceDirectory !== null && sourceDirectory !== ".") { const relative = index.get(`${sourceDirectory}/${partialName}`) - if (relative !== undefined) return relative + if (relative !== undefined) return pickForCaller(relative, sourceFile) } if (!partialName.includes("/")) { const application = index.get(`${APPLICATION_DIRECTORY}/${partialName}`) - if (application !== undefined) return application + if (application !== undefined) return pickForCaller(application, sourceFile) } return null diff --git a/javascript/packages/analysis/src/project-index.ts b/javascript/packages/analysis/src/project-index.ts index baf60e444..d491e0e9b 100644 --- a/javascript/packages/analysis/src/project-index.ts +++ b/javascript/packages/analysis/src/project-index.ts @@ -58,8 +58,8 @@ export class ProjectIndex { return this.callerIndex } - get viewRoot(): string | undefined { - return this.partialIndex?.viewRoot + get viewRoots(): string[] | undefined { + return this.partialIndex?.viewRoots } async indexAll(): Promise { @@ -71,7 +71,7 @@ export class ProjectIndex { try { this.partialIndex = await buildPartialIndex(this.backend, this.root) - this.logger?.log(`[Partials] Indexed ${this.partialIndex.size} partials under ${this.partialIndex.viewRoot}`) + this.logger?.log(`[Partials] Indexed ${this.partialIndex.size} partials under ${this.partialIndex.viewRoots.join(", ")}`) } catch (error) { this.logger?.warn(`[Partials] Failed to index partials: ${this.messageFor(error)}`) } diff --git a/javascript/packages/analysis/src/render-graph-builder.ts b/javascript/packages/analysis/src/render-graph-builder.ts index bbc6431f5..1c36ad3c9 100644 --- a/javascript/packages/analysis/src/render-graph-builder.ts +++ b/javascript/packages/analysis/src/render-graph-builder.ts @@ -6,7 +6,7 @@ import { readFileSync } from "node:fs" import { getTagLocalName, isERBCaseNode, isERBIfNode, isERBOutputNode, isERBRenderNode, isERBUnlessNode, isHTMLElementNode, isPrismNodeType, isRubyRenderLocalNode } from "@herb-tools/core" import { outranksTemplate } from "./partial-index" -import { layoutCandidatesFor, templateNameForFile, isPartialPath } from "./partial-resolution" +import { layoutCandidatesForRoots, templateNameForRoots, isPartialPath } from "./partial-resolution" import { renderPartialExpression } from "./render-expression" import { staticAncestorAttributes } from "./ancestor-attributes" @@ -227,11 +227,11 @@ export function collectCallSites(herb: HerbBackend, partials: PartialIndex, file return { unresolved, isDocumentRoot, yields, roots: { ...roots, renders: rootRenders, resolved: rootsResolved } } } -function addLayoutCallSites(files: string[], layoutYields: Map, viewRoot: string, callSites: Map): void { +function addLayoutCallSites(files: string[], layoutYields: Map, viewRoots: string[], callSites: Map): void { const layouts = new Map() for (const file of files) { - const name = templateNameForFile(file, viewRoot) + const name = templateNameForRoots(file, viewRoots) if (name === null || !layoutYields.has(file)) { continue @@ -247,7 +247,7 @@ function addLayoutCallSites(files: string[], layoutYields: Map { - const files = await templatesIn(projectPath, partials.viewRoot, options.include ?? []) + const files = await templatesIn(projectPath, partials.viewRoots[0] ?? ".", options.include ?? []) const excluded = options.exclude?.length ? picomatch(options.exclude) : null const callSites = new Map() const documentRoots = new Set() @@ -318,7 +318,7 @@ export async function buildRenderGraph(herb: HerbBackend, projectPath: string, p } if (options.resolveLayouts !== false) { - addLayoutCallSites(scanned, layoutYields, partials.viewRoot, callSites) + addLayoutCallSites(scanned, layoutYields, partials.viewRoots, callSites) } return new RenderGraph(callSites, roots, documentRoots, unresolvedRenders, skippedFiles) diff --git a/javascript/packages/analysis/test/partial-index-builder.test.ts b/javascript/packages/analysis/test/partial-index-builder.test.ts index afed4e34d..74f0f38e9 100644 --- a/javascript/packages/analysis/test/partial-index-builder.test.ts +++ b/javascript/packages/analysis/test/partial-index-builder.test.ts @@ -84,7 +84,7 @@ describe("buildPartialIndex", () => { const index = await buildPartialIndex(Herb, root) - expect(index.viewRoot).toBe("app/views") + expect(index.viewRoots).toEqual(["app/views"]) expect(index.size).toBe(2) expect(index.lookup("users/card", "app/views/posts/index.html.erb")).toEqual({ @@ -126,7 +126,7 @@ describe("buildPartialIndex", () => { const index = await buildPartialIndex(Herb, root) - expect(index.viewRoot).toBe(".") + expect(index.viewRoots).toEqual(["."]) expect(index.lookup("views/card", "index.html.erb")?.file).toBe("views/_card.html.erb") }) diff --git a/javascript/packages/analysis/test/partial-index.test.ts b/javascript/packages/analysis/test/partial-index.test.ts index b708507ac..cc2b0ad86 100644 --- a/javascript/packages/analysis/test/partial-index.test.ts +++ b/javascript/packages/analysis/test/partial-index.test.ts @@ -15,7 +15,7 @@ beforeAll(async () => { }) describe("PartialIndex", () => { - const index = new PartialIndex("app/views", new Map([ + const index = new PartialIndex(["app/views"], new Map([ ["users/card", declaration("app/views/users/_card.html.erb", [{ name: "user", required: true }])], ["application/flash", declaration("app/views/application/_flash.html.erb", [{ name: "message", required: true }])], ])) @@ -43,7 +43,7 @@ describe("PartialIndex", () => { test("round trips through its serialized form", () => { const restored = PartialIndex.from(index.toJSON()) - expect(restored.viewRoot).toBe("app/views") + expect(restored.viewRoots).toEqual(["app/views"]) expect(restored.size).toBe(2) expect(restored.lookup("users/card", "app/views/posts/index.html.erb")?.locals).toEqual([{ name: "user", required: true }]) }) @@ -57,7 +57,7 @@ describe("PartialIndex", () => { describe("PartialIndex updates", () => { function index(): PartialIndex { - return new PartialIndex("app/views", new Map([ + return new PartialIndex(["app/views"], new Map([ ["users/card", declaration("app/views/users/_card.html.erb", [{ name: "user", required: true }])], ])) } @@ -120,7 +120,7 @@ describe("PartialIndex updates", () => { }) test("lets the base template take over from a variant", () => { - const partials = new PartialIndex("app/views", new Map([ + const partials = new PartialIndex(["app/views"], new Map([ ["users/card", declaration("app/views/users/_card.en.html.erb", [{ name: "user", required: true }])], ])) @@ -130,7 +130,7 @@ describe("PartialIndex updates", () => { }) test("forgets the displaced variant when the base template takes over", () => { - const partials = new PartialIndex("app/views", new Map([ + const partials = new PartialIndex(["app/views"], new Map([ ["users/card", declaration("app/views/users/_card.html+phone.erb", [])], ])) diff --git a/javascript/packages/analysis/test/partial-resolution.test.ts b/javascript/packages/analysis/test/partial-resolution.test.ts index 1e5f9ff12..89ccb0bcc 100644 --- a/javascript/packages/analysis/test/partial-resolution.test.ts +++ b/javascript/packages/analysis/test/partial-resolution.test.ts @@ -1,6 +1,8 @@ import { describe, test, expect } from "vitest" import { + formatOf, + variantOf, isPartialPath, partialNameForFile, resolvePartial, @@ -126,45 +128,118 @@ describe("@herb-tools/core", () => { ]) test("resolves a fully qualified name", () => { - expect(resolvePartial("users/card", "app/views/posts/index.html.erb", index, VIEW_ROOT)).toBe("app/views/users/_card.html.erb") + expect(resolvePartial("users/card", "app/views/posts/index.html.erb", index, [VIEW_ROOT])).toBe("app/views/users/_card.html.erb") }) test("resolves a bare name against the rendering template's directory", () => { - expect(resolvePartial("avatar", "app/views/users/show.html.erb", index, VIEW_ROOT)).toBe("app/views/users/_avatar.html.erb") + expect(resolvePartial("avatar", "app/views/users/show.html.erb", index, [VIEW_ROOT])).toBe("app/views/users/_avatar.html.erb") }) test("falls back to the application directory for a bare name", () => { - expect(resolvePartial("flash", "app/views/posts/index.html.erb", index, VIEW_ROOT)).toBe("app/views/application/_flash.html.erb") + expect(resolvePartial("flash", "app/views/posts/index.html.erb", index, [VIEW_ROOT])).toBe("app/views/application/_flash.html.erb") }) test("prefers the exact name over the relative one", () => { - expect(resolvePartial("users/card", "app/views/admin/index.html.erb", index, VIEW_ROOT)).toBe("app/views/users/_card.html.erb") + expect(resolvePartial("users/card", "app/views/admin/index.html.erb", index, [VIEW_ROOT])).toBe("app/views/users/_card.html.erb") }) test("resolves a qualified name relative to the rendering template's directory", () => { const nested = paths(["app/views/admin/users/_card.html.erb"]) - expect(resolvePartial("users/card", "app/views/admin/index.html.erb", nested, VIEW_ROOT)).toBe("app/views/admin/users/_card.html.erb") + expect(resolvePartial("users/card", "app/views/admin/index.html.erb", nested, [VIEW_ROOT])).toBe("app/views/admin/users/_card.html.erb") }) test("does not fall back to the application directory for a qualified name", () => { - expect(resolvePartial("users/flash", "app/views/posts/index.html.erb", index, VIEW_ROOT)).toBeNull() + expect(resolvePartial("users/flash", "app/views/posts/index.html.erb", index, [VIEW_ROOT])).toBeNull() }) test("returns null for an unknown partial", () => { - expect(resolvePartial("users/missing", "app/views/posts/index.html.erb", index, VIEW_ROOT)).toBeNull() + expect(resolvePartial("users/missing", "app/views/posts/index.html.erb", index, [VIEW_ROOT])).toBeNull() }) test("resolves from a template at the view root", () => { - expect(resolvePartial("flash", "app/views/index.html.erb", index, VIEW_ROOT)).toBe("app/views/application/_flash.html.erb") + expect(resolvePartial("flash", "app/views/index.html.erb", index, [VIEW_ROOT])).toBe("app/views/application/_flash.html.erb") }) test("resolves from a source file outside the view root", () => { - expect(resolvePartial("users/card", "app/components/card_component.html.erb", index, VIEW_ROOT)).toBe("app/views/users/_card.html.erb") + expect(resolvePartial("users/card", "app/components/card_component.html.erb", index, [VIEW_ROOT])).toBe("app/views/users/_card.html.erb") }) test("resolves without a known source file", () => { - expect(resolvePartial("users/card", "", index, VIEW_ROOT)).toBe("app/views/users/_card.html.erb") + expect(resolvePartial("users/card", "", index, [VIEW_ROOT])).toBe("app/views/users/_card.html.erb") }) }) }) + +describe("formatOf", () => { + test("reads the format out of a filename", () => { + expect(formatOf("app/views/posts/_row.html.erb")).toBe("html") + expect(formatOf("app/views/posts/_row.turbo_stream.erb")).toBe("turbo_stream") + expect(formatOf("app/views/posts/_row.html.herb")).toBe("html") + expect(formatOf("app/views/posts/_row.en.html.erb")).toBe("html") + }) + + test("returns null when the filename carries no format", () => { + expect(formatOf("app/views/posts/_row.erb")).toBeNull() + expect(formatOf("app/views/posts/_row.herb")).toBeNull() + }) +}) + +describe("variantOf", () => { + test("reads the variant out of a filename", () => { + expect(variantOf("app/views/posts/_row.html+mobile.erb")).toBe("mobile") + expect(variantOf("app/views/posts/_row.html+tablet.herb")).toBe("tablet") + }) + + test("returns null when the filename carries no variant", () => { + expect(variantOf("app/views/posts/_row.html.erb")).toBeNull() + expect(variantOf("app/views/posts/_row.erb")).toBeNull() + }) + + test("a variant keeps the format of its base template", () => { + expect(formatOf("app/views/posts/_row.html+mobile.erb")).toBe("html") + expect(formatOf("app/views/posts/_row.turbo_stream+mobile.erb")).toBe("turbo_stream") + }) +}) + +describe("format-aware resolution", () => { + const HTML_CALLER = "app/views/posts/index.html.erb" + const TURBO_CALLER = "app/views/posts/index.turbo_stream.erb" + + test("a caller reaches the partial matching its own format", () => { + const index: PartialPaths = new Map([ + ["posts/row", ["app/views/posts/_row.html.erb", "app/views/posts/_row.turbo_stream.erb"]], + ]) + + expect(resolvePartial("posts/row", HTML_CALLER, index, [VIEW_ROOT])).toBe("app/views/posts/_row.html.erb") + expect(resolvePartial("posts/row", TURBO_CALLER, index, [VIEW_ROOT])).toBe("app/views/posts/_row.turbo_stream.erb") + }) + + test("a formatless partial serves any caller", () => { + const index: PartialPaths = new Map([["posts/row", ["app/views/posts/_row.erb"]]]) + + expect(resolvePartial("posts/row", TURBO_CALLER, index, [VIEW_ROOT])).toBe("app/views/posts/_row.erb") + }) + + test("a formatless partial loses to an exact format match", () => { + const index: PartialPaths = new Map([ + ["posts/row", ["app/views/posts/_row.erb", "app/views/posts/_row.turbo_stream.erb"]], + ]) + + expect(resolvePartial("posts/row", TURBO_CALLER, index, [VIEW_ROOT])).toBe("app/views/posts/_row.turbo_stream.erb") + }) + + test("the plain template is preferred over a variant", () => { + const index: PartialPaths = new Map([ + ["posts/row", ["app/views/posts/_row.html+mobile.erb", "app/views/posts/_row.html.erb"]], + ]) + + expect(resolvePartial("posts/row", HTML_CALLER, index, [VIEW_ROOT])).toBe("app/views/posts/_row.html.erb") + }) + + test("a single file still resolves", () => { + const index: PartialPaths = new Map([["posts/row", "app/views/posts/_row.html.erb"]]) + + expect(resolvePartial("posts/row", HTML_CALLER, index, [VIEW_ROOT])).toBe("app/views/posts/_row.html.erb") + }) +}) diff --git a/javascript/packages/analysis/test/project-index.test.ts b/javascript/packages/analysis/test/project-index.test.ts index 910dcb46f..a18cdfe1e 100644 --- a/javascript/packages/analysis/test/project-index.test.ts +++ b/javascript/packages/analysis/test/project-index.test.ts @@ -64,13 +64,13 @@ describe("ProjectIndex", () => { test("reports the view root it found", async () => { const index = await analyzerFor() - expect(index.viewRoot).toBe("app/views") + expect(index.viewRoots).toEqual(["app/views"]) }) test("falls back to the project root when there is no app/views", async () => { const index = await analyzerFor({ "templates/_card.html.erb": `
\n` }) - expect(index.viewRoot).toBe(".") + expect(index.viewRoots).toEqual(["."]) }) }) diff --git a/javascript/packages/analysis/test/view-roots.test.ts b/javascript/packages/analysis/test/view-roots.test.ts new file mode 100644 index 000000000..bbfe77ec8 --- /dev/null +++ b/javascript/packages/analysis/test/view-roots.test.ts @@ -0,0 +1,83 @@ +import { describe, test, expect } from "vitest" + +import { PartialIndex } from "../src/partial-index" + +import { + partialNameForRoots, + relativeToViewRoots, + resolvePartial, + rootIndexFor, + templateNameForRoots, +} from "../src/partial-resolution" + +import type { PartialPaths } from "../src/partial-resolution" + +const APP = "app/views" +const ENGINE = "engines/billing/app/views" +const ROOTS = [APP, ENGINE] + +describe("relativeToViewRoots", () => { + test("returns the first root that contains the file", () => { + expect(relativeToViewRoots(`${APP}/home/index.html.erb`, ROOTS)).toEqual([0, "home/index.html.erb"]) + expect(relativeToViewRoots(`${ENGINE}/billing/_invoice.html.erb`, ROOTS)).toEqual([1, "billing/_invoice.html.erb"]) + }) + + test("returns null when no root contains the file", () => { + expect(relativeToViewRoots("lib/elsewhere/_thing.html.erb", ROOTS)).toBeNull() + }) +}) + +describe("partialNameForRoots", () => { + test("names a partial from a secondary view root", () => { + expect(partialNameForRoots(`${ENGINE}/billing/_invoice.html.erb`, ROOTS)).toBe("billing/invoice") + }) + + test("names a partial from the primary view root", () => { + expect(partialNameForRoots(`${APP}/shared/_header.html.erb`, ROOTS)).toBe("shared/header") + }) + + test("returns null for a file outside every root", () => { + expect(partialNameForRoots("lib/_thing.html.erb", ROOTS)).toBeNull() + }) +}) + +describe("templateNameForRoots", () => { + test("names a template from a secondary view root", () => { + expect(templateNameForRoots(`${ENGINE}/billing/index.html.erb`, ROOTS)).toBe("billing/index") + }) +}) + +describe("rootIndexFor", () => { + test("orders an earlier view root ahead of a later one", () => { + expect(rootIndexFor(`${APP}/billing/_invoice.html.erb`, ROOTS)).toBe(0) + expect(rootIndexFor(`${ENGINE}/billing/_invoice.html.erb`, ROOTS)).toBe(1) + }) + + test("sorts an unknown file last", () => { + expect(rootIndexFor("lib/_thing.html.erb", ROOTS)).toBe(ROOTS.length) + }) +}) + +describe("resolvePartial", () => { + test("resolves a sibling within the root that owns the caller", () => { + const index: PartialPaths = new Map([["billing/row", `${ENGINE}/billing/_row.html.erb`]]) + const caller = `${ENGINE}/billing/index.html.erb` + + expect(resolvePartial("row", caller, index, ROOTS)).toBe(`${ENGINE}/billing/_row.html.erb`) + }) + + test("resolves with a single root", () => { + const index: PartialPaths = new Map([["shared/header", `${APP}/shared/_header.html.erb`]]) + + expect(resolvePartial("shared/header", "", index, [APP])).toBe(`${APP}/shared/_header.html.erb`) + }) +}) + +describe("serialization", () => { + test("round-trips every view root", () => { + const index = new PartialIndex(ROOTS, new Map()) + const restored = PartialIndex.from(index.toJSON()) + + expect(restored.viewRoots).toEqual(ROOTS) + }) +}) diff --git a/javascript/packages/language-server/src/session.ts b/javascript/packages/language-server/src/session.ts index dd238bd9c..5a31f6ca0 100644 --- a/javascript/packages/language-server/src/session.ts +++ b/javascript/packages/language-server/src/session.ts @@ -105,7 +105,7 @@ export class Session { private viewRootFor(documentPath: string): string | null { const project = this.projects.containing(documentPath) - const viewRoot = project?.index.viewRoot + const viewRoot = project?.index.viewRoots?.[0] if (!project || viewRoot === undefined) return null diff --git a/javascript/packages/language-service/src/completion_provider.ts b/javascript/packages/language-service/src/completion_provider.ts index bf28a3f96..246346dcd 100644 --- a/javascript/packages/language-service/src/completion_provider.ts +++ b/javascript/packages/language-service/src/completion_provider.ts @@ -464,7 +464,7 @@ export class CompletionProvider { if (!partials) return null const file = this.relativePathFor(document.uri) - const directory = file === null ? null : this.directoryOf(file, partials.viewRoot) + const directory = file === null ? null : this.directoryOf(file, partials.viewRoots[0] ?? ".") const lowercasePrefix = prefix.toLowerCase() const nameRange = Range.create(document.positionAt(document.offsetAt(position) - prefix.length), position) diff --git a/javascript/packages/linter/src/rules/actionview-prefer-qualified-partial-path.ts b/javascript/packages/linter/src/rules/actionview-prefer-qualified-partial-path.ts index 8f1b73cb6..9867b7414 100644 --- a/javascript/packages/linter/src/rules/actionview-prefer-qualified-partial-path.ts +++ b/javascript/packages/linter/src/rules/actionview-prefer-qualified-partial-path.ts @@ -3,7 +3,7 @@ import { ParserRule } from "../types.js" import { renderPartialExpression } from "@herb-tools/analysis" import { isERBOutputNode, isPrismNodeType, locationFromByteOffset, substringFromByteOffset } from "@herb-tools/core" -import { partialNameForFile } from "@herb-tools/analysis" +import { partialNameForRoots } from "@herb-tools/analysis" import type { ERBRenderNode, ParseResult, ParserOptions, PrismNode } from "@herb-tools/core" import type { BaseAutofixContext, FullRuleConfig, LintContext, LintOffense, Mutable, UnboundLintOffense } from "../types.js" @@ -106,7 +106,7 @@ class ActionViewPreferQualifiedPartialPathVisitor extends BaseRuleVisitor { }) describe("declaration frames", () => { - const located = new PartialIndex("app/views", new Map([ + const located = new PartialIndex(["app/views"], new Map([ ["users/card", { ...declaration("app/views/users/_card.html.erb", [{ name: "user", required: true }]), location: { line: 1, column: 0 } }], ["users/plain", declaration("app/views/users/_plain.html.erb", [{ name: "user", required: true }])], ])) diff --git a/javascript/packages/linter/test/rules/actionview-prefer-qualified-partial-path.test.ts b/javascript/packages/linter/test/rules/actionview-prefer-qualified-partial-path.test.ts index b10f17d61..6c917b5f6 100644 --- a/javascript/packages/linter/test/rules/actionview-prefer-qualified-partial-path.test.ts +++ b/javascript/packages/linter/test/rules/actionview-prefer-qualified-partial-path.test.ts @@ -13,7 +13,7 @@ function declaration(file: string): PartialDeclaration { return { file, hasDeclaration: false, hasKeywordRest: false, locals: [] } } -const partials = new PartialIndex("app/views", new Map([ +const partials = new PartialIndex(["app/views"], new Map([ ["posts/card", declaration("app/views/posts/_card.html.erb")], ["application/flash", declaration("app/views/application/_flash.html.erb")], ])) @@ -70,7 +70,7 @@ describe("actionview-prefer-qualified-partial-path", () => { test("falls back to the generic advice when the partial does not resolve", () => { expectInfo(GENERIC) - assertOffenses(`<%= render "card" %>`, { fileName: "app/views/nowhere/index.html.erb", partials: new PartialIndex("app/views", new Map()) }) + assertOffenses(`<%= render "card" %>`, { fileName: "app/views/nowhere/index.html.erb", partials: new PartialIndex(["app/views"], new Map()) }) }) test("does not flag a qualified path in the shorthand form", () => { diff --git a/lib/herb/analysis/partial_index.rb b/lib/herb/analysis/partial_index.rb index cc4ad92c8..3f88a98a4 100644 --- a/lib/herb/analysis/partial_index.rb +++ b/lib/herb/analysis/partial_index.rb @@ -10,8 +10,6 @@ module Analysis class PartialIndex APPLICATION_DIRECTORY = "application" #: String - attr_reader :view_root #: Pathname - attr_reader :templates #: Array[String] #: (String | Pathname, ?templates: Array[String]?) -> PartialIndex @@ -20,7 +18,7 @@ def self.build(project_path, templates: nil) view_root = resolve_view_root(root) files = templates || Dir[view_root.join("**", PartialResolution::TEMPLATE_GLOB_PATTERN)].sort - new(view_root, files) + new([view_root], files) end #: (String | Pathname) -> Pathname @@ -28,14 +26,17 @@ def self.resolve_view_root(project_path) PartialResolution.view_root_for(project_path) end - #: (String | Pathname, Array[String]) -> void - def initialize(view_root, templates) - @view_root = Pathname.new(view_root) + #: (Array[String | Pathname], Array[String]) -> void + def initialize(view_roots, templates) + @view_roots = view_roots.map { |root| Pathname.new(root) } #: Array[Pathname] @templates = templates @by_name = build_index(templates) @declarations = {} #: Hash[String, PartialDeclaration?] end + #: () -> Array[Pathname] + attr_reader :view_roots + #: (String?) -> Array[String] def files_for(partial_name) return [] unless partial_name @@ -43,10 +44,34 @@ def files_for(partial_name) @by_name[partial_name] || [] end + #: (String?, String?) -> Array[String] #: (String?, String?) -> Array[String] def resolve(partial_name, source_file) + candidates = candidates_for(partial_name, source_file) + format = source_file ? PartialResolution.format_of(source_file) : nil + + return candidates unless format + + candidates.sort_by do |file| + candidate = PartialResolution.format_of(file) + + matches = if candidate == format + 0 + elsif candidate.nil? + 1 + else + 2 + end + + [matches, PartialResolution.variant_of(file) ? 1 : 0, PartialResolution.has_locale?(file) ? 1 : 0] + end + end + + #: (String?, String?) -> Array[String] + def candidates_for(partial_name, source_file) return [] unless partial_name + partial_name = PartialResolution.without_template_extension(partial_name) exact = files_for(partial_name) return exact if exact.any? @@ -73,7 +98,7 @@ def self.partial_name_for(file, view_root) #: (String) -> String? def partial_name_for(file) - self.class.partial_name_for(file, @view_root) + PartialResolution.partial_name_for_roots(file, @view_roots) end #: () -> Array[String] @@ -105,7 +130,9 @@ def update(file) @templates = (@templates | [file]).sort files = (@by_name[name] || []) | [file] - @by_name[name] = PartialResolution.by_precedence(files) + ordered = PartialResolution.by_precedence(files) + + @by_name[name] = ordered.sort_by { |candidate| PartialResolution.root_index_for(candidate, @view_roots) } name end @@ -152,7 +179,10 @@ def to_h #: (String) -> String? def source_directory_for(source_file) - Pathname.new(File.dirname(source_file)).relative_path_from(@view_root).to_s + directory = Pathname.new(File.dirname(source_file)) + root = @view_roots.find { |candidate| directory.to_s.start_with?(candidate.to_s) } || @view_root + + directory.relative_path_from(root).to_s rescue ArgumentError nil end @@ -181,7 +211,11 @@ def build_index(files) (map[name] ||= []) << file end - map.each_value { |files| files.replace(PartialResolution.by_precedence(files)) } + map.each_value do |candidates| + ordered = PartialResolution.by_precedence(candidates) + candidates.replace(ordered.sort_by { |file| PartialResolution.root_index_for(file, @view_roots) }) + end + map end end diff --git a/lib/herb/analysis/partial_resolution.rb b/lib/herb/analysis/partial_resolution.rb index 071c671d2..eca7af85c 100644 --- a/lib/herb/analysis/partial_resolution.rb +++ b/lib/herb/analysis/partial_resolution.rb @@ -41,6 +41,65 @@ def view_root_for(project_path) candidates.find(&:directory?) || root end + #: (String) -> String? + def format_of(file) + base = File.basename(file) + dot = base.index(".") + + return nil unless dot + + extension = base[dot..].to_s + stripped = extension.delete_suffix(".erb") + stripped = stripped.delete_suffix(".herb") if stripped == extension + + return nil if stripped == extension + + format = stripped.delete_prefix(".").split(".").last.to_s.split("+").first.to_s + + format.empty? ? nil : format + end + + #: (String) -> bool + def has_locale?(file) + base = File.basename(file) + dot = base.index(".") + + return false unless dot + + extension = base[dot..].to_s + stripped = extension.delete_suffix(".erb") + stripped = stripped.delete_suffix(".herb") if stripped == extension + + return false if stripped == extension + + stripped.delete_prefix(".").split(".").size > 1 + end + + #: (String) -> String? + def variant_of(file) + base = File.basename(file) + dot = base.index(".") + + return nil unless dot + + extension = base[dot..].to_s + stripped = extension.delete_suffix(".erb") + stripped = stripped.delete_suffix(".herb") if stripped == extension + + return nil if stripped == extension + + _, variant = stripped.split("+", 2) + + variant.to_s.empty? ? nil : variant + end + + #: (String) -> String + def without_template_extension(partial_name) + extension = EXTENSIONS.find { |candidate| partial_name.end_with?(candidate) } + + extension ? partial_name.delete_suffix(extension) : partial_name + end + #: (String) -> bool def template_path?(file) name = File.basename(file) @@ -100,6 +159,11 @@ def template_name_for(file, view_root) directory == "." ? name : "#{directory}/#{name}" end + #: (String, Array[String | Pathname]) -> Array[String] + def layout_candidates_for_roots(template_file, view_roots) + view_roots.lazy.map { |root| layout_candidates_for(template_file, root) }.find { |candidates| candidates.any? } || [] + end + #: (String, String | Pathname) -> Array[String] def layout_candidates_for(template_file, view_root) relative = relative_to_view_root(template_file, view_root) @@ -142,6 +206,34 @@ def partial_name_for(file, view_root) directory == "." ? name : "#{directory}/#{name}" end + #: (String, Array[String | Pathname]) -> [Integer, String]? + def relative_to_view_roots(file, view_roots) + view_roots.each_with_index do |root, index| + relative = relative_to_view_root(file, root) + + return [index, relative] if relative + end + + nil + end + + #: (String, Array[String | Pathname]) -> String? + def partial_name_for_roots(file, view_roots) + view_roots.filter_map { |root| partial_name_for(file, root) }.first + end + + #: (String, Array[String | Pathname]) -> String? + def template_name_for_roots(file, view_roots) + view_roots.filter_map { |root| template_name_for(file, root) }.first + end + + #: (String, Array[String | Pathname]) -> Integer + def root_index_for(file, view_roots) + found = relative_to_view_roots(file, view_roots) + + found ? found[0] : view_roots.size + end + private #: (String, String | Pathname) -> String? diff --git a/lib/herb/analysis/project_index.rb b/lib/herb/analysis/project_index.rb index 998d9a2aa..1f88053de 100644 --- a/lib/herb/analysis/project_index.rb +++ b/lib/herb/analysis/project_index.rb @@ -44,9 +44,9 @@ def index_call_sites @graph = graph_builder.build(partials.templates) end - #: () -> Pathname? - def view_root - @partials&.view_root + #: () -> Array[Pathname]? + def view_roots + @partials&.view_roots end #: (String, ?String?) -> bool diff --git a/lib/herb/analysis/render_analyzer.rb b/lib/herb/analysis/render_analyzer.rb index 3f07ce9b6..cf72d68f4 100644 --- a/lib/herb/analysis/render_analyzer.rb +++ b/lib/herb/analysis/render_analyzer.rb @@ -438,6 +438,36 @@ def analyze_from_collected(render_calls_by_file:, dynamic_prefixes_from_erb: [], end def print_file_lists(result) + formatless = result.partial_files.values.select { |file| missing_format?(file) }.sort + + if formatless.any? + puts "\n" + puts " #{bold("Templates without a format:")}" + puts " #{dimmed("Rails reads a template filename as `name.format.handler`. Without a format it matches every one.")}" + puts "" + + formatless.each do |file| + puts " #{bold(yellow("!"))} #{yellow(relative_path(file))}" + end + end + + conditional = conditional_calls(result) + + if conditional.any? + puts "\n" + puts " #{bold("Conditional render calls:")}" + puts " #{dimmed("The partial name is chosen at runtime, but every branch is a literal.")}" + puts "" + + conditional.each do |call, targets| + puts " #{bold(yellow("?"))} #{call[:partial]} #{dimmed("in #{relative_path(call[:file])}")}" + + targets.each do |target| + puts " #{dimmed("\u2192")} #{green(relative_path(target))}" + end + end + end + return unless result.issues? if result.unresolved.any? @@ -456,7 +486,10 @@ def print_file_lists(result) calls.each do |call| location = call[:location] ? dimmed("at #{call[:location]}") : nil expected = expected_file_path(call[:partial], result.view_root) - puts " #{bold(red("\u2717"))} #{bold(call[:partial])} #{location} #{dimmed("-")} #{dimmed(expected)}" + kind = render_name_kind(call[:partial]) + label = kind ? " #{dimmed("(#{kind})")}" : "" + + puts " #{bold(red("\u2717"))} #{bold(call[:partial])}#{label} #{location} #{dimmed("-")} #{dimmed(expected)}" end end end @@ -465,13 +498,17 @@ def print_file_lists(result) puts "\n #{separator}" if result.unresolved.any? puts "\n" puts " #{bold("Dynamic render calls:")}" - puts " #{dimmed("The partial name is built at runtime, so it cannot be resolved statically.")}" + puts " #{dimmed("The partial name is built at runtime. Where the directory is known, every partial under it is listed.")}" puts "" result.dynamic_calls.each do |call| shown = dynamic_call_display(call) puts " #{bold(red("\u2717"))} #{bold(red(shown))} #{dimmed("in #{relative_path(call[:file])}")}" + + names_under(call, result.partial_files).each do |name| + puts " #{dimmed("\u2192")} #{dimmed(name)}" + end end end @@ -526,7 +563,7 @@ def print_summary_line(result) #: (String) -> bool def component_template?(relative) - relative.start_with?("app/components/") + relative.start_with?("app/components/") || relative.include?("/app/components/") end #: (Array[Hash[Symbol, untyped]]) -> void @@ -541,7 +578,7 @@ def print_warning_summary_line(warnings) locals = files_for.call(:undeclared_local) uninferable = files_for.call(:uninferable_local) ivars = files_for.call(:ivar_in_partial) - ignored = files_for.call(:ignored_component) + ignored = find_erb_files.map { |file| relative_path(file) }.count { |file| component_template?(file) } parts = [] #: Array[String] parts << stat(locals, "undeclared #{pluralize(locals, "local")}", :yellow) if locals.positive? @@ -555,7 +592,7 @@ def print_warning_summary_line(warnings) return unless ignored.positive? - puts " #{label("Ignored")} #{dimmed("#{ignored} component #{pluralize(ignored, "template")} in app/components/")}" + puts " #{label("Ignored")} #{dimmed("#{ignored} component #{pluralize(ignored, "template")} under app/components/")}" end private @@ -1053,11 +1090,22 @@ def build_render_graph(render_calls_by_file, partial_files, view_root) next unless partial_reference resolved = resolve_partial(partial_reference, file, partial_files, view_root) + if resolved resolved_name = partial_name_for_file(resolved, view_root) resolved_names << resolved_name if resolved_name else - resolved_names << partial_reference + branches = static_branches(partial_reference) + targets = branches.filter_map { |branch| resolve_partial(branch, file, partial_files, view_root) } + + if branches.size > 1 && targets.size == branches.size + targets.each do |target| + name = partial_name_for_file(target, view_root) + resolved_names << name if name + end + else + resolved_names << partial_reference + end end end @@ -1100,6 +1148,13 @@ def find_unused_by_reachability(render_graph, partial_files, ruby_references, dy queue << resolved_file if resolved_file end + partial_files.each do |name, file| + next unless dynamic_prefixes.any? { |prefix| name.start_with?("#{prefix}/") } + + reachable << name + queue << file if file + end + visited_files = Set.new until queue.empty? @@ -1154,7 +1209,7 @@ def collect_all_dynamic_prefixes(dynamic_calls, ruby_references) prefix = call[:partial].gsub(/\A["']|["']\z/, "") prefix = prefix.split("\#{").first&.chomp("/") - prefix unless prefix.nil? || prefix.empty? + prefix if prefix && !prefix.empty? && partial_path_segment?(prefix) } ruby_references.each do |reference| @@ -1168,8 +1223,12 @@ def collect_all_dynamic_prefixes(dynamic_calls, ruby_references) def find_unresolved(render_calls, partial_files, view_root) render_calls.select do |call| next false unless call[:partial] + next false if resolve_partial(call[:partial], call[:file], partial_files, view_root) + + branches = static_branches(call[:partial]) + targets = branches.filter_map { |branch| resolve_partial(branch, call[:file], partial_files, view_root) } - !resolve_partial(call[:partial], call[:file], partial_files, view_root) + !(branches.size > 1 && targets.size == branches.size) end end @@ -1177,6 +1236,63 @@ def resolve_partial(partial_name, source_file, _partial_files, view_root) partial_index(view_root).resolve(partial_name, source_file).first end + #: (untyped) -> Array[[Hash[Symbol, untyped], Array[String]]] + def conditional_calls(result) + result.render_calls.filter_map do |call| + next unless call[:partial] + next if resolve_partial(call[:partial], call[:file], result.partial_files, result.view_root) + + branches = static_branches(call[:partial]) + targets = branches.filter_map { |branch| resolve_partial(branch, call[:file], result.partial_files, result.view_root) } + + [call, targets] if branches.size > 1 && targets.size == branches.size + end + end + + #: (Hash[Symbol, untyped], Hash[String, String]) -> Array[String] + def names_under(call, partial_files) + prefix = call[:dynamic_prefix] + + unless prefix + raw = call[:partial].to_s.sub(/\A["']/, "") + prefix = raw.split("\#{").first.to_s.chomp("/") + end + + return [] if prefix.to_s.empty? || !partial_path_segment?(prefix) + + partial_files.keys.select { |name| name.start_with?("#{prefix}/") }.sort + end + + #: (String) -> bool + def missing_format?(file) + name = File.basename(file) + + name.end_with?(".erb") && name.count(".") == 1 + end + + #: (String) -> bool + def partial_path_segment?(value) + value.match?(%r{\A[a-z0-9_/-]+\z}) + end + + #: (String) -> Array[String] + def static_branches(expression) + branches = expression.split("?", 2).last.to_s + + branches.scan(/["']([^"'\#]+)["']/).flatten.uniq + end + + #: (String) -> String? + def render_name_kind(name) + return "instance variable" if name.start_with?("@") + return "interpolated" if name.include?("\#{") + return "conditional" if name.include?("?") && name.include?(":") + return "method call" if name.include?("(") || name.include?(".") + return "expression" if name.include?(" ") + + nil + end + def expected_file_path(partial_name, view_root) parts = partial_name.split("/") parts[-1] = "_#{parts[-1]}" @@ -1223,6 +1339,10 @@ def format_duration(seconds) end def relative_path(path) + relative = Pathname.new(path).relative_path_from(@project_path).to_s + + return relative unless relative.start_with?("..") + Pathname.new(path).relative_path_from(Pathname.pwd).to_s rescue ArgumentError path.to_s diff --git a/lib/herb/analysis/render_graph/builder.rb b/lib/herb/analysis/render_graph/builder.rb index 45ccb83ce..99a19b1da 100644 --- a/lib/herb/analysis/render_graph/builder.rb +++ b/lib/herb/analysis/render_graph/builder.rb @@ -156,11 +156,11 @@ def build(templates) #: (Array[String], Hash[String, Array[YieldSite]], Hash[String, Array[PartialCallSite]]) -> void def add_layout_call_sites(files, layout_yields, call_sites) - view_root = @partials.view_root + view_roots = @partials.view_roots layouts = {} #: Hash[String, String] files.each do |file| - name = PartialResolution.template_name_for(file, view_root) + name = PartialResolution.template_name_for_roots(file, view_roots) next unless name && layout_yields.key?(file) @@ -172,7 +172,7 @@ def add_layout_call_sites(files, layout_yields, call_sites) end files.each do |file| - PartialResolution.layout_candidates_for(file, view_root).each do |candidate| + PartialResolution.layout_candidates_for_roots(file, view_roots).each do |candidate| layout = layouts[candidate] next if layout.nil? || layout == file diff --git a/lib/herb/analysis/ruby_locals_index.rb b/lib/herb/analysis/ruby_locals_index.rb index cebb92c7b..da5749e54 100644 --- a/lib/herb/analysis/ruby_locals_index.rb +++ b/lib/herb/analysis/ruby_locals_index.rb @@ -59,7 +59,6 @@ def find(name) @locals.find { |local| local.name == name } end - # Every name the template binds, regardless of where. #: () -> Set[String] def names @locals.to_set(&:name) diff --git a/lib/herb/analysis/ruby_locals_index/named_reference.rb b/lib/herb/analysis/ruby_locals_index/named_reference.rb index 9bed92e89..5468e0567 100644 --- a/lib/herb/analysis/ruby_locals_index/named_reference.rb +++ b/lib/herb/analysis/ruby_locals_index/named_reference.rb @@ -3,8 +3,6 @@ module Herb module Analysis class RubyLocalsIndex - # A name, with where it appears in the source as a byte offset and length, - # which is how Prism reports it. class NamedReference attr_reader :name #: String attr_reader :start_offset #: Integer diff --git a/lib/herb/analysis/ruby_locals_index/offset_table.rb b/lib/herb/analysis/ruby_locals_index/offset_table.rb index fdab6e800..4a3178bf7 100644 --- a/lib/herb/analysis/ruby_locals_index/offset_table.rb +++ b/lib/herb/analysis/ruby_locals_index/offset_table.rb @@ -3,8 +3,6 @@ module Herb module Analysis class RubyLocalsIndex - # Prism reports byte offsets into the whole template while the Herb AST - # reports lines and columns, so one of them has to be translated. class OffsetTable # @rbs! # @line_starts: Array[Integer] diff --git a/lib/herb/analysis/template_dependencies.rb b/lib/herb/analysis/template_dependencies.rb index 597c82b0c..af74fd65f 100644 --- a/lib/herb/analysis/template_dependencies.rb +++ b/lib/herb/analysis/template_dependencies.rb @@ -100,7 +100,6 @@ def dependency_index(file_path) end # @rbs! - # KERNEL_METHODS: Array[String] KERNEL_METHODS = [ "rand", "srand", "format", "sprintf", "raise", "loop", "sleep", "catch", "throw", "block_given?", "caller", "binding", "frozen?", "freeze", "dup", "clone", "tap", "then", @@ -340,7 +339,6 @@ def symbol_after(line, keyword) end # @rbs! - # UNCOUNTABLE: Array[String] UNCOUNTABLE = ["series", "species", "news", "information", "equipment", "money"].freeze #: (String, String) -> String? @@ -376,7 +374,7 @@ def trace_state(entry_point, state) return nil unless entry_result return nil unless entry_result.instance_variables.include?(state) || entry_result.constants.include?(state) - index = PartialIndex.new(@view_root, reachable) + index = PartialIndex.new([@view_root], reachable) affected = Set.new([entry_point]) #: Set[String] state_locals = {} #: Hash[String, Set[String]] diff --git a/rust/herb-analysis/src/actionview_cli.rs b/rust/herb-analysis/src/actionview_cli.rs index 84c9a2516..91a774d62 100644 --- a/rust/herb-analysis/src/actionview_cli.rs +++ b/rust/herb-analysis/src/actionview_cli.rs @@ -9,7 +9,52 @@ use herb_analysis::render_graph::Verdict; use herb_analysis::ruby_render_references; use herb_analysis::state_flow::{FlowNode, StateFlow}; +fn actionview_configured(project_path: &Path) -> Result<(), String> { + let Ok(config) = herb_config::Config::load(project_path, None) else { + return Ok(()); + }; + + match config.config.framework { + Some(herb_config::Framework::ActionView) => Ok(()), + Some(other) => Err(format!("{other:?}").to_lowercase()), + None => Err("ruby".to_string()), + } +} + +fn report_missing_framework(project_path: &Path, framework: &str) -> i32 { + println!(); + println!( + " {}", + "Herb also works outside of ActionView, but the `herb actionview` commands require the project to be explicitly configured for ActionView.".dimmed() + ); + println!(); + println!( + " The project at '{}' is not configured to use ActionView (current framework: '{framework}').", + project_path.display() + ); + println!(); + println!(" To enable ActionView support, add the following to your `.herb.yml`:"); + println!(); + println!(" {}", "framework: actionview".bold()); + println!(); + + 1 +} + pub fn run(command: &str, arguments: &[String]) -> i32 { + if !matches!(command, "check" | "graph" | "dependencies" | "flow" | "context" | "signature") { + eprintln!("{}", format!("Unknown actionview subcommand: {command}").red()); + print_usage(); + + return 1; + } + + let root = project_root(arguments); + + if let Err(framework) = actionview_configured(&root) { + return report_missing_framework(&root, &framework); + } + match command { "check" => check(arguments), "graph" => graph(arguments), @@ -90,6 +135,68 @@ fn header(title: &str) { println!(); } +fn component_template(relative: &str) -> bool { + relative.starts_with("app/components/") || relative.contains("/app/components/") +} + +fn missing_format(file: &str) -> bool { + let name = file.rsplit('/').next().unwrap_or(file); + + name.ends_with(".erb") && name.matches('.').count() == 1 +} + +fn static_branches(expression: &str) -> Vec { + let mut found: Vec = Vec::new(); + + let mut rest = match expression.split_once('?') { + Some((_, branches)) => branches, + None => expression, + }; + + while let Some(start) = rest.find(['"', '\'']) { + let quote = rest.as_bytes()[start] as char; + let after = &rest[start + 1..]; + + let Some(end) = after.find(quote) else { + break; + }; + + let literal = &after[..end]; + + if !literal.is_empty() && !literal.contains("#{") && !found.iter().any(|seen| seen == literal) { + found.push(literal.to_string()); + } + + rest = &after[end + 1..]; + } + + found +} + +fn render_name_kind(name: &str) -> Option<&'static str> { + if name.starts_with('@') { + return Some("instance variable"); + } + + if name.contains("#{") { + return Some("interpolated"); + } + + if name.contains('?') && name.contains(':') { + return Some("conditional"); + } + + if name.contains('(') || name.contains('.') { + return Some("method call"); + } + + if name.contains(' ') { + return Some("expression"); + } + + None +} + fn plural(count: usize, word: &str) -> String { if count == 1 { word.to_string() @@ -155,7 +262,8 @@ fn check(arguments: &[String]) -> i32 { let mut rendered: Vec = Vec::new(); let mut files_with_renders: BTreeSet = BTreeSet::new(); let mut dynamic_renders = 0usize; - let mut dynamic_sites: Vec<(String, String)> = Vec::new(); + let mut dynamic_sites: Vec<(String, String, Vec)> = Vec::new(); + let mut branching_sites: Vec<(String, String, Vec)> = Vec::new(); let mut other_renders = 0usize; let mut with_partial_count = 0usize; @@ -187,6 +295,8 @@ fn check(arguments: &[String]) -> i32 { for call in &result.render_calls { files_with_renders.insert(file.clone()); + let guessed = call.partial.is_none() && call.layout.is_none(); + let target = call .partial .clone() @@ -205,7 +315,13 @@ fn check(arguments: &[String]) -> i32 { .map(|prefix| format!("{prefix}/#{{...}}")) .unwrap_or_else(|| "#{...}".to_string()); - dynamic_sites.push((relative(file, &root), shown)); + let candidates = call + .dynamic_prefix + .as_ref() + .map(|prefix| index.names_under(prefix).iter().map(|name| (*name).to_string()).collect()) + .unwrap_or_default(); + + dynamic_sites.push((relative(file, &root), shown, candidates)); } continue; @@ -223,13 +339,44 @@ fn check(arguments: &[String]) -> i32 { eprintln!("{file}\t{name}"); } - match index.resolve(name, Some(file)).first() { - Some(target) => rendered.push(target.clone()), - None => unresolved.push((relative(file, &root), name.clone())), + let resolved = index.resolve(name, Some(file)); + + match resolved.first() { + Some(target) => { + rendered.push(target.clone()); + + let format = herb_analysis::partial_resolution::format_of(target); + + for candidate in resolved.iter().skip(1) { + if herb_analysis::partial_resolution::variant_of(candidate).is_some() && herb_analysis::partial_resolution::format_of(candidate) == format { + rendered.push(candidate.clone()); + } + } + } + None => { + let branches = static_branches(name); + let targets: Vec = branches + .iter() + .filter_map(|branch| index.resolve(branch, Some(file)).first().cloned()) + .collect(); + + if branches.len() > 1 && targets.len() == branches.len() { + rendered.extend(targets.iter().cloned()); + branching_sites.push(( + relative(file, &root), + name.clone(), + targets.iter().map(|target| relative(target, &root)).collect(), + )); + } else if !guessed { + unresolved.push((relative(file, &root), name.clone())); + } + } } } } + let formatless: Vec = templates.iter().filter(|file| missing_format(file)).map(|file| relative(file, &root)).collect(); + let partials: Vec = templates .iter() .filter(|file| herb_analysis::partial_resolution::partial_path(file)) @@ -270,12 +417,45 @@ fn check(arguments: &[String]) -> i32 { println!(); + if !formatless.is_empty() { + println!(" {}", "Templates without a format:".bold()); + println!( + " {}", + "Rails reads a template filename as `name.format.handler`. Without a format it matches every one.".dimmed() + ); + println!(); + + for file in &formatless { + println!(" {} {}", "!".yellow().bold(), file.yellow()); + } + + println!(); + } + + if !branching_sites.is_empty() { + println!(" {}", "Conditional render calls:".bold()); + println!(" {}", "The partial name is chosen at runtime, but every branch is a literal.".dimmed()); + println!(); + + for (file, expression, targets) in &branching_sites { + println!(" {} {} {}", "?".yellow().bold(), expression, format!("in {file}").dimmed()); + + for target in targets { + println!(" {} {}", "\u{2192}".dimmed(), target.green()); + } + } + + println!(); + } + if !unresolved.is_empty() { println!(" {}", "Unresolved render calls:".bold()); println!(); for (file, name) in &unresolved { - println!(" {} {} {}", "\u{2717}".red().bold(), name, format!("in {file}").dimmed()); + let kind = render_name_kind(name).map(|kind| format!(" ({kind})").dimmed().to_string()).unwrap_or_default(); + + println!(" {} {}{} {}", "\u{2717}".red().bold(), name, kind, format!("in {file}").dimmed()); } println!(); @@ -283,11 +463,18 @@ fn check(arguments: &[String]) -> i32 { if !dynamic_sites.is_empty() { println!(" {}", "Dynamic render calls:".bold()); - println!(" {}", "The partial name is built at runtime, so it cannot be resolved statically.".dimmed()); + println!( + " {}", + "The partial name is built at runtime. Where the directory is known, every partial under it is listed.".dimmed() + ); println!(); - for (file, shown) in &dynamic_sites { + for (file, shown, candidates) in &dynamic_sites { println!(" {} {} {}", "\u{2717}".red().bold(), shown.red().bold(), format!("in {file}").dimmed()); + + for candidate in candidates { + println!(" {} {}", "\u{2192}".dimmed(), candidate.dimmed()); + } } println!(); @@ -403,7 +590,11 @@ fn check(arguments: &[String]) -> i32 { println!( " {} {}", label("Ignored"), - format!("{ignored_components} component {} in app/components/", plural(ignored_components, "template")).dimmed() + format!( + "{ignored_components} component {} under app/components/", + plural(ignored_components, "template") + ) + .dimmed() ); } @@ -579,10 +770,14 @@ fn graph(arguments: &[String]) -> i32 { } fn view_relative(file: &str, index: &PartialIndex) -> String { - Path::new(file) - .strip_prefix(index.view_root()) + let path = Path::new(file); + + index + .view_roots() + .iter() + .find_map(|root| path.strip_prefix(root).ok()) .map(|rest| rest.display().to_string()) - .unwrap_or_else(|_| file.to_string()) + .unwrap_or_else(|| file.to_string()) } fn reverse_graph(renders: &BTreeMap>, index: &PartialIndex) -> BTreeMap> { @@ -679,7 +874,7 @@ fn collect_renders(index: &mut PartialIndex, templates: &[String]) -> (BTreeMap< let mut renders: BTreeMap> = BTreeMap::new(); let mut prefixes: BTreeSet = BTreeSet::new(); let mut layouts: BTreeSet = BTreeSet::new(); - let flow = StateFlow::new(index.view_root()); + let flow = StateFlow::new(index.view_roots().first().map(PathBuf::as_path).unwrap_or_else(|| Path::new("."))); for file in templates { let result = flow.analyze(file); @@ -763,7 +958,8 @@ fn reachable_partials( continue; } - let Some(file) = index.resolve(&name, None).first() else { + let resolved = index.resolve(&name, None); + let Some(file) = resolved.first() else { continue; }; @@ -1289,7 +1485,11 @@ fn print_dependency_warnings( let mut unknown: Vec<(String, Vec)> = Vec::new(); let mut likely_locals: Vec<(String, Vec)> = Vec::new(); let mut uninferable: Vec<(String, Vec)> = Vec::new(); - let mut ignored_components = 0usize; + let ignored_components = templates + .iter() + .map(|file| relative(file, root)) + .filter(|file| component_template(file)) + .count(); for file in templates { let result = flow.analyze(file); @@ -1315,8 +1515,9 @@ fn print_dependency_warnings( } if !rest.is_empty() { - if relative.starts_with("app/components/") { - ignored_components += 1; + if component_template(&relative) { + // Counted from the template list instead, so the total does not depend on how many + // helpers each binding happens to resolve. } else if partial && !declared && candidates.is_none_or(BTreeSet::is_empty) { uninferable.push((relative, rest)); } else { diff --git a/rust/herb-analysis/src/partial_index.rs b/rust/herb-analysis/src/partial_index.rs index 099ead5f2..ba3a2f25f 100644 --- a/rust/herb-analysis/src/partial_index.rs +++ b/rust/herb-analysis/src/partial_index.rs @@ -5,10 +5,10 @@ use std::path::{Path, PathBuf}; use herb::herb::{parse_with_options, ParserOptions}; use crate::partial_declaration::PartialDeclaration; -use crate::partial_resolution::{self, by_precedence, partial_name_for, template_path, view_root_for, APPLICATION_DIRECTORY}; +use crate::partial_resolution::{self, by_precedence, partial_name_for_roots, root_index_for, template_path, view_root_for, APPLICATION_DIRECTORY}; pub struct PartialIndex { - view_root: PathBuf, + view_roots: Vec, templates: Vec, by_name: BTreeMap>, declarations: BTreeMap, @@ -40,9 +40,16 @@ impl PartialIndex { let files = config.find_files_for_tool(herb_config::Tool::Linter, Some(project_path)); if !files.is_empty() { - let templates: Vec = files.into_iter().filter(|file| crate::partial_resolution::template_path(file)).collect(); + let mut templates: Vec = files.into_iter().filter(|file| crate::partial_resolution::template_path(file)).collect(); if !templates.is_empty() { + let known: std::collections::BTreeSet<&String> = templates.iter().collect(); + let extra: Vec = index.templates.iter().filter(|file| !known.contains(file)).cloned().collect(); + + templates.extend(extra); + templates.sort(); + templates.dedup(); + index.replace_templates(templates); } } @@ -58,16 +65,16 @@ impl PartialIndex { collect_templates(&view_root, &mut templates); templates.sort(); - Self::new(&view_root, templates) + Self::new(&[view_root], templates) } pub fn resolve_view_root(project_path: &Path) -> PathBuf { view_root_for(project_path) } - pub fn new(view_root: &Path, templates: Vec) -> Self { + pub fn new(view_roots: &[PathBuf], templates: Vec) -> Self { let mut index = Self { - view_root: view_root.to_path_buf(), + view_roots: view_roots.to_vec(), templates, by_name: BTreeMap::new(), declarations: BTreeMap::new(), @@ -77,6 +84,10 @@ impl PartialIndex { index } + fn root_strings(&self) -> Vec { + self.view_roots.iter().filter_map(|root| root.to_str().map(str::to_string)).collect() + } + fn rebuild(&mut self) { let mut by_name: BTreeMap> = BTreeMap::new(); @@ -88,15 +99,18 @@ impl PartialIndex { by_name.entry(name).or_default().push(file.clone()); } + let roots = self.root_strings(); + for files in by_name.values_mut() { by_precedence(files); + files.sort_by_key(|file| root_index_for(file, &roots)); } self.by_name = by_name; } - pub fn view_root(&self) -> &Path { - &self.view_root + pub fn view_roots(&self) -> &[PathBuf] { + &self.view_roots } pub fn templates(&self) -> &[String] { @@ -107,6 +121,12 @@ impl PartialIndex { self.by_name.keys().map(|name| name.as_str()).collect() } + pub fn names_under(&self, prefix: &str) -> Vec<&str> { + let prefix = format!("{}/", prefix.trim_end_matches('/')); + + self.by_name.keys().filter(|name| name.starts_with(&prefix)).map(|name| name.as_str()).collect() + } + pub fn to_h(&mut self) -> BTreeMap { let names: Vec = self.names().iter().map(|name| name.to_string()).collect(); let mut partials = BTreeMap::new(); @@ -131,7 +151,7 @@ impl PartialIndex { } pub fn partial_name_for(&self, file: &str) -> Option { - partial_name_for(file, self.view_root.to_str()?) + partial_name_for_roots(file, &self.root_strings()) } pub fn files_for(&self, partial_name: &str) -> &[String] { @@ -140,12 +160,41 @@ impl PartialIndex { fn source_directory_for(&self, source_file: &str) -> Option { let directory = Path::new(source_file).parent()?; - let relative = directory.strip_prefix(&self.view_root).ok()?; - Some(relative.to_str()?.to_string()) + self + .view_roots + .iter() + .find_map(|root| directory.strip_prefix(root).ok()) + .and_then(|relative| relative.to_str()) + .map(str::to_string) + } + + pub fn resolve(&self, partial_name: &str, source_file: Option<&str>) -> Vec { + let candidates = self.candidates(partial_name, source_file); + let Some(format) = source_file.and_then(partial_resolution::format_of) else { + return candidates.to_vec(); + }; + + let mut ordered = candidates.to_vec(); + ordered.sort_by_key(|file| { + let matches = match partial_resolution::format_of(file) { + Some(candidate) if candidate == format => 0, + None => 1, + Some(_) => 2, + }; + + ( + matches, + usize::from(partial_resolution::variant_of(file).is_some()), + usize::from(partial_resolution::has_locale(file)), + ) + }); + + ordered } - pub fn resolve(&self, partial_name: &str, source_file: Option<&str>) -> &[String] { + fn candidates(&self, partial_name: &str, source_file: Option<&str>) -> &[String] { + let partial_name = partial_resolution::without_template_extension(partial_name); let exact = self.files_for(partial_name); if !exact.is_empty() { diff --git a/rust/herb-analysis/src/partial_resolution.rs b/rust/herb-analysis/src/partial_resolution.rs index 448e35d6a..96d905746 100644 --- a/rust/herb-analysis/src/partial_resolution.rs +++ b/rust/herb-analysis/src/partial_resolution.rs @@ -56,6 +56,56 @@ fn normalize(path: &str) -> String { } } +pub fn format_of(file: &str) -> Option { + let normalized = normalize(file); + let base = basename(&normalized); + let dot = base.find('.')?; + let extension = &base[dot..]; + + let stripped = extension.strip_suffix(".erb").or_else(|| extension.strip_suffix(".herb"))?; + let segments = stripped.strip_prefix('.')?; + let format = segments.rsplit('.').next().unwrap_or(segments); + let format = format.split('+').next().unwrap_or(format); + + (!format.is_empty()).then(|| format.to_string()) +} + +pub fn has_locale(file: &str) -> bool { + let normalized = normalize(file); + let base = basename(&normalized); + + let Some(dot) = base.find('.') else { + return false; + }; + + let extension = &base[dot..]; + + let Some(stripped) = extension.strip_suffix(".erb").or_else(|| extension.strip_suffix(".herb")) else { + return false; + }; + + stripped.strip_prefix('.').is_some_and(|segments| segments.split('.').count() > 1) +} + +pub fn variant_of(file: &str) -> Option { + let normalized = normalize(file); + let base = basename(&normalized); + let dot = base.find('.')?; + let extension = &base[dot..]; + + let stripped = extension.strip_suffix(".erb").or_else(|| extension.strip_suffix(".herb"))?; + let (_, variant) = stripped.rsplit_once('+')?; + + (!variant.is_empty()).then(|| variant.to_string()) +} + +pub fn without_template_extension(partial_name: &str) -> &str { + EXTENSIONS + .iter() + .find_map(|extension| partial_name.strip_suffix(extension)) + .unwrap_or(partial_name) +} + pub fn template_path(file: &str) -> bool { let normalized = normalize(file); let name = basename(&normalized); @@ -70,6 +120,13 @@ pub fn partial_path(file: &str) -> bool { name.starts_with(PARTIAL_PREFIX) && EXTENSIONS.iter().any(|extension| name.ends_with(extension)) } +pub fn relative_to_view_roots(path: &str, view_roots: &[String]) -> Option<(usize, String)> { + view_roots + .iter() + .enumerate() + .find_map(|(index, root)| relative_to_view_root(path, root).map(|relative| (index, relative))) +} + fn relative_to_view_root(path: &str, view_root: &str) -> Option { let normalized_path = normalize(path); let normalized_root = normalize(view_root); @@ -94,6 +151,18 @@ fn without_extension(name: &str) -> &str { } } +pub fn partial_name_for_roots(file: &str, view_roots: &[String]) -> Option { + view_roots.iter().find_map(|root| partial_name_for(file, root)) +} + +pub fn template_name_for_roots(file: &str, view_roots: &[String]) -> Option { + view_roots.iter().find_map(|root| template_name_for(file, root)) +} + +pub fn root_index_for(file: &str, view_roots: &[String]) -> usize { + relative_to_view_roots(file, view_roots).map(|(index, _)| index).unwrap_or(view_roots.len()) +} + pub fn partial_name_for(file: &str, view_root: &str) -> Option { if !partial_path(file) { return None; @@ -148,6 +217,14 @@ pub fn template_name_for(file: &str, view_root: &str) -> Option { } } +pub fn layout_candidates_for_roots(template_file: &str, view_roots: &[String]) -> Vec { + view_roots + .iter() + .map(|root| layout_candidates_for(template_file, root)) + .find(|candidates| !candidates.is_empty()) + .unwrap_or_default() +} + pub fn layout_candidates_for(template_file: &str, view_root: &str) -> Vec { let Some(relative) = relative_to_view_root(template_file, view_root) else { return Vec::new(); diff --git a/rust/herb-analysis/src/project_index.rs b/rust/herb-analysis/src/project_index.rs index ccece30d9..a41d30867 100644 --- a/rust/herb-analysis/src/project_index.rs +++ b/rust/herb-analysis/src/project_index.rs @@ -66,8 +66,8 @@ impl ProjectIndex { self.graph.as_ref() } - pub fn view_root(&self) -> Option<&Path> { - self.partials.as_ref().map(|partials| partials.view_root()) + pub fn view_roots(&self) -> Option<&[PathBuf]> { + self.partials.as_ref().map(|partials| partials.view_roots()) } pub fn handle_change(&mut self, path: &str, source: Option<&str>) -> bool { diff --git a/rust/herb-analysis/src/render_graph_builder.rs b/rust/herb-analysis/src/render_graph_builder.rs index 0ffa130c4..3cc4ef4fc 100644 --- a/rust/herb-analysis/src/render_graph_builder.rs +++ b/rust/herb-analysis/src/render_graph_builder.rs @@ -6,7 +6,7 @@ use herb::nodes::{AnyNode, ERBCaseNode, ERBIfNode, ERBRenderNode, ERBUnlessNode, use herb::visitor::Visitor; use crate::partial_index::PartialIndex; -use crate::partial_resolution::{layout_candidates_for, outranks_template, partial_path, template_name_for, LAYOUTS_DIRECTORY}; +use crate::partial_resolution::{layout_candidates_for_roots, outranks_template, partial_path, template_name_for_roots, LAYOUTS_DIRECTORY}; use crate::render_graph::{CallSiteLocation, PartialCallSite, RenderGraph, StaticAttributeMap, TemplateRoots}; const RENDER_MARKER: &str = "render"; @@ -179,14 +179,12 @@ impl<'a> Builder<'a> { } fn add_layout_call_sites(&self, files: &[String], layout_yields: &BTreeMap>, graph: &mut RenderGraph) { - let Some(view_root) = self.partials.view_root().to_str() else { - return; - }; + let view_roots: Vec = self.partials.view_roots().iter().filter_map(|root| root.to_str().map(str::to_string)).collect(); let mut layouts: BTreeMap = BTreeMap::new(); for file in files { - let Some(name) = template_name_for(file, view_root) else { + let Some(name) = template_name_for_roots(file, &view_roots) else { continue; }; @@ -203,7 +201,7 @@ impl<'a> Builder<'a> { } for file in files { - for candidate in layout_candidates_for(file, view_root) { + for candidate in layout_candidates_for_roots(file, &view_roots) { let Some(layout) = layouts.get(&candidate) else { continue; }; diff --git a/rust/herb-analysis/src/ruby_render_references.rs b/rust/herb-analysis/src/ruby_render_references.rs index 1f2b301fa..01c4a13b6 100644 --- a/rust/herb-analysis/src/ruby_render_references.rs +++ b/rust/herb-analysis/src/ruby_render_references.rs @@ -74,6 +74,7 @@ pub fn collect_from_source(source: &str, references: &mut RubyRenderReferences) let wrapped = format!("<% {} %>", source); let options = ParserOptions { prism_nodes: true, + prism_program: true, ..Default::default() }; @@ -81,6 +82,12 @@ pub fn collect_from_source(source: &str, references: &mut RubyRenderReferences) return; }; + if let Some(program) = result.value.prism() { + walk(program, references); + + return; + } + for child in &result.value.children { if let herb::nodes::AnyNode::ERBContentNode(node) = child { if let Some(prism) = node.prism() { diff --git a/rust/herb-analysis/src/template_dependencies.rs b/rust/herb-analysis/src/template_dependencies.rs index c325a02a1..358dd5df1 100644 --- a/rust/herb-analysis/src/template_dependencies.rs +++ b/rust/herb-analysis/src/template_dependencies.rs @@ -361,13 +361,19 @@ fn dynamic_prefix_of(value: &str) -> Option { let value = value.trim_start_matches(['"', '\'']); let head = value.split("#{").next()?.trim_end_matches('/'); - if head.is_empty() { + if head.is_empty() || !partial_path_segment(head) { None } else { Some(head.to_string()) } } +fn partial_path_segment(value: &str) -> bool { + value + .chars() + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_' || c == '/' || c == '-') +} + fn interpolated_render_prefix(node: &herb::prism::PrismNode) -> Option { if node.is("InterpolatedStringNode") { let first = node.children.first()?; diff --git a/rust/herb-analysis/tests/actionview_cli_test.rs b/rust/herb-analysis/tests/actionview_cli_test.rs index 5b7b4e769..d895e7258 100644 --- a/rust/herb-analysis/tests/actionview_cli_test.rs +++ b/rust/herb-analysis/tests/actionview_cli_test.rs @@ -15,6 +15,7 @@ impl Project { let _ = fs::remove_dir_all(&root); fs::create_dir_all(root.join("app/views/posts")).expect("create project"); fs::create_dir_all(root.join("app/views/layouts")).expect("create layouts"); + fs::write(root.join(".herb.yml"), "framework: actionview\n").expect("configure project"); Self { root } } diff --git a/rust/herb-analysis/tests/formats_test.rs b/rust/herb-analysis/tests/formats_test.rs new file mode 100644 index 000000000..28c33bd48 --- /dev/null +++ b/rust/herb-analysis/tests/formats_test.rs @@ -0,0 +1,119 @@ +use std::fs; +use std::path::PathBuf; + +use herb_analysis::partial_index::PartialIndex; +use herb_analysis::partial_resolution::{format_of, variant_of}; + +fn scratch(name: &str) -> PathBuf { + let root = std::env::temp_dir().join(format!("herb-formats-{name}")); + + let _ = fs::remove_dir_all(&root); + + root +} + +fn write(path: &PathBuf) -> String { + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, "
\n").unwrap(); + + path.to_str().unwrap().to_string() +} + +#[test] +fn reads_the_format_out_of_a_filename() { + assert_eq!(Some("html".to_string()), format_of("app/views/posts/_row.html.erb")); + assert_eq!(Some("turbo_stream".to_string()), format_of("app/views/posts/_row.turbo_stream.erb")); + assert_eq!(Some("html".to_string()), format_of("app/views/posts/_row.html.herb")); + assert_eq!(None, format_of("app/views/posts/_row.erb")); + assert_eq!(None, format_of("app/views/posts/_row.herb")); + assert_eq!(Some("html".to_string()), format_of("app/views/posts/_row.en.html.erb")); +} + +#[test] +fn a_caller_reaches_the_partial_matching_its_own_format() { + let root = scratch("matching"); + let views = root.join("app/views"); + + let html_caller = write(&views.join("posts/index.html.erb")); + let turbo_caller = write(&views.join("posts/index.turbo_stream.erb")); + let html_partial = write(&views.join("posts/_row.html.erb")); + let turbo_partial = write(&views.join("posts/_row.turbo_stream.erb")); + + let index = PartialIndex::new( + &[views], + vec![html_caller.clone(), turbo_caller.clone(), html_partial.clone(), turbo_partial.clone()], + ); + + assert_eq!(html_partial, index.resolve("posts/row", Some(&html_caller))[0]); + assert_eq!(turbo_partial, index.resolve("posts/row", Some(&turbo_caller))[0]); +} + +#[test] +fn a_formatless_partial_serves_any_caller() { + let root = scratch("formatless"); + let views = root.join("app/views"); + + let turbo_caller = write(&views.join("posts/index.turbo_stream.erb")); + let partial = write(&views.join("posts/_row.erb")); + + let index = PartialIndex::new(&[views], vec![turbo_caller.clone(), partial.clone()]); + + assert_eq!(partial, index.resolve("posts/row", Some(&turbo_caller))[0]); +} + +#[test] +fn a_formatless_partial_loses_to_an_exact_format_match() { + let root = scratch("exact-wins"); + let views = root.join("app/views"); + + let turbo_caller = write(&views.join("posts/index.turbo_stream.erb")); + let formatless = write(&views.join("posts/_row.erb")); + let turbo_partial = write(&views.join("posts/_row.turbo_stream.erb")); + + let index = PartialIndex::new(&[views], vec![turbo_caller.clone(), formatless, turbo_partial.clone()]); + + assert_eq!(turbo_partial, index.resolve("posts/row", Some(&turbo_caller))[0]); +} + +#[test] +fn extension_precedence_still_decides_when_no_format_matches() { + let root = scratch("fallback"); + let views = root.join("app/views"); + + let turbo_caller = write(&views.join("posts/index.turbo_stream.erb")); + let html_partial = write(&views.join("posts/_row.html.erb")); + + let index = PartialIndex::new(&[views], vec![turbo_caller.clone(), html_partial.clone()]); + + assert_eq!(html_partial, index.resolve("posts/row", Some(&turbo_caller))[0]); +} + +#[test] +fn reads_the_variant_out_of_a_filename() { + assert_eq!(Some("mobile".to_string()), variant_of("app/views/posts/_row.html+mobile.erb")); + assert_eq!(Some("tablet".to_string()), variant_of("app/views/posts/_row.html+tablet.herb")); + assert_eq!(None, variant_of("app/views/posts/_row.html.erb")); + assert_eq!(None, variant_of("app/views/posts/_row.erb")); +} + +#[test] +fn a_variant_keeps_the_format_of_its_base_template() { + assert_eq!(Some("html".to_string()), format_of("app/views/posts/_row.html+mobile.erb")); + assert_eq!(Some("turbo_stream".to_string()), format_of("app/views/posts/_row.turbo_stream+mobile.erb")); +} + +#[test] +fn the_plain_template_is_preferred_over_a_variant() { + let root = scratch("variant"); + let views = root.join("app/views"); + + let caller = write(&views.join("posts/index.html.erb")); + let variant = write(&views.join("posts/_row.html+mobile.erb")); + let plain = write(&views.join("posts/_row.html.erb")); + + let index = PartialIndex::new(&[views], vec![caller.clone(), variant.clone(), plain.clone()]); + let resolved = index.resolve("posts/row", Some(&caller)); + + assert_eq!(plain, resolved[0]); + assert!(resolved.contains(&variant), "the variant is still reachable: {resolved:?}"); +} diff --git a/rust/herb-analysis/tests/object_render_test.rs b/rust/herb-analysis/tests/object_render_test.rs new file mode 100644 index 000000000..1dc76fe74 --- /dev/null +++ b/rust/herb-analysis/tests/object_render_test.rs @@ -0,0 +1,91 @@ +#![cfg(feature = "cli")] + +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::Command; + +fn binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_herb-analysis")) +} + +fn scratch(name: &str) -> PathBuf { + let root = std::env::temp_dir().join(format!("herb-object-render-{name}")); + + let _ = fs::remove_dir_all(&root); + + root +} + +fn write(root: &Path, relative: &str, body: &str) { + let path = root.join(relative); + + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, body).unwrap(); + fs::write(root.join(".herb.yml"), "framework: actionview\n").unwrap(); +} + +fn check(root: &Path) -> String { + let output = Command::new(binary()).args(["actionview", "check", root.to_str().unwrap()]).output().unwrap(); + + String::from_utf8_lossy(&output.stdout).to_string() +} + +#[test] +fn a_guessed_object_partial_that_does_not_exist_is_not_an_error() { + let root = scratch("miss"); + + write(&root, "app/views/components/index.html.erb", "<%= render body do %><% end %>\n"); + + let output = check(&root); + + assert!(!output.contains("bodys/body"), "a guessed name should not be reported as unresolved:\n{output}"); +} + +#[test] +fn a_named_partial_that_does_not_exist_is_still_an_error() { + let root = scratch("named"); + + write(&root, "app/views/posts/index.html.erb", "<%= render \"posts/missing\" %>\n"); + + let output = check(&root); + + assert!(output.contains("posts/missing"), "an explicit name should still be reported:\n{output}"); +} + +#[test] +fn a_guessed_object_partial_that_exists_still_resolves() { + let root = scratch("hit"); + + write(&root, "app/views/posts/index.html.erb", "<%= render post %>\n"); + write(&root, "app/views/posts/_post.html.erb", "
\n"); + + let output = check(&root); + + assert!(!output.contains("posts/post"), "an existing guessed target should resolve:\n{output}"); +} + +#[test] +fn a_dynamic_render_lists_the_partials_under_its_prefix() { + let root = scratch("dynamic-prefix"); + + write(&root, "app/views/admin/show.html.erb", "<%= render \"admin/parts/#{name}\" %>\n"); + write(&root, "app/views/admin/parts/_alpha.html.erb", "
\n"); + write(&root, "app/views/admin/parts/_beta.html.erb", "
\n"); + + let output = check(&root); + + assert!(output.contains("admin/parts/alpha"), "candidates should be listed:\n{output}"); + assert!(output.contains("admin/parts/beta"), "candidates should be listed:\n{output}"); +} + +#[test] +fn a_dynamic_render_with_no_known_directory_lists_nothing() { + let root = scratch("dynamic-bare"); + + write(&root, "app/views/admin/show.html.erb", "<%= render \"#{name}\" %>\n"); + write(&root, "app/views/admin/parts/_alpha.html.erb", "
\n"); + + let output = check(&root); + + assert!(!output.contains("admin/parts/alpha"), "nothing should be claimed:\n{output}"); +} diff --git a/rust/herb-analysis/tests/partial_index_test.rs b/rust/herb-analysis/tests/partial_index_test.rs index 58815b729..6e156678e 100644 --- a/rust/herb-analysis/tests/partial_index_test.rs +++ b/rust/herb-analysis/tests/partial_index_test.rs @@ -1,5 +1,5 @@ use std::fs; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use herb_analysis::partial_index::PartialIndex; @@ -51,7 +51,7 @@ fn resolves_the_view_root_to_app_views_when_it_is_there() { let project = Project::new("view_root"); project.write("app/views/posts/index.html.erb"); - assert_eq!(project.index().view_root(), project.root.join("app/views")); + assert_eq!(project.index().view_roots(), [project.root.join("app/views")]); } #[test] @@ -59,7 +59,7 @@ fn falls_back_to_the_project_root_when_there_is_no_app_views() { let project = Project::new("flat_root"); project.write("posts/index.html.erb"); - assert_eq!(project.index().view_root(), Path::new(&project.root)); + assert_eq!(project.index().view_roots(), [project.root.as_path()].map(PathBuf::from)); } #[test] diff --git a/rust/herb-analysis/tests/project_index_test.rs b/rust/herb-analysis/tests/project_index_test.rs index 42cb672ed..0ed433992 100644 --- a/rust/herb-analysis/tests/project_index_test.rs +++ b/rust/herb-analysis/tests/project_index_test.rs @@ -151,5 +151,5 @@ fn exposes_the_view_root_it_resolved() { let project = Project::new("view_root"); project.write("app/views/posts/index.html.erb", "
"); - assert_eq!(project.indexed().view_root().expect("view root"), project.root.join("app/views")); + assert_eq!(project.indexed().view_roots().expect("view roots"), [project.root.join("app/views")]); } diff --git a/rust/herb-analysis/tests/view_roots_test.rs b/rust/herb-analysis/tests/view_roots_test.rs new file mode 100644 index 000000000..f7c8b2ed5 --- /dev/null +++ b/rust/herb-analysis/tests/view_roots_test.rs @@ -0,0 +1,91 @@ +use std::fs; +use std::path::PathBuf; + +use herb_analysis::partial_index::PartialIndex; + +fn scratch(name: &str) -> PathBuf { + let root = std::env::temp_dir().join(format!("herb-view-roots-{name}")); + + let _ = fs::remove_dir_all(&root); + + root +} + +fn write(path: &PathBuf, body: &str) { + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, body).unwrap(); +} + +#[test] +fn names_a_partial_from_a_secondary_view_root() { + let root = scratch("secondary"); + let app = root.join("app/views"); + let engine = root.join("engines/billing/app/views"); + + write(&app.join("home/index.html.erb"), "
\n"); + write(&engine.join("billing/_invoice.html.erb"), "
\n"); + + let templates = vec![ + app.join("home/index.html.erb").to_str().unwrap().to_string(), + engine.join("billing/_invoice.html.erb").to_str().unwrap().to_string(), + ]; + + let index = PartialIndex::new(&[app.clone(), engine.clone()], templates); + + assert_eq!(vec!["billing/invoice"], index.names()); +} + +#[test] +fn an_earlier_view_root_shadows_a_later_one() { + let root = scratch("shadow"); + let app = root.join("app/views"); + let engine = root.join("engines/billing/app/views"); + + write(&app.join("billing/_invoice.html.erb"), "
app
\n"); + write(&engine.join("billing/_invoice.html.erb"), "
engine
\n"); + + let templates = vec![ + engine.join("billing/_invoice.html.erb").to_str().unwrap().to_string(), + app.join("billing/_invoice.html.erb").to_str().unwrap().to_string(), + ]; + + let index = PartialIndex::new(&[app.clone(), engine.clone()], templates); + let resolved = index.resolve("billing/invoice", None); + + assert_eq!(2, resolved.len()); + assert!(resolved[0].starts_with(app.to_str().unwrap()), "app view path should win, got {}", resolved[0]); +} + +#[test] +fn resolves_a_sibling_within_the_root_that_owns_the_caller() { + let root = scratch("sibling"); + let app = root.join("app/views"); + let engine = root.join("engines/billing/app/views"); + + write(&engine.join("billing/index.html.erb"), "
\n"); + write(&engine.join("billing/_row.html.erb"), "
\n"); + + let templates = vec![ + engine.join("billing/index.html.erb").to_str().unwrap().to_string(), + engine.join("billing/_row.html.erb").to_str().unwrap().to_string(), + ]; + + let index = PartialIndex::new(&[app, engine.clone()], templates); + let caller = engine.join("billing/index.html.erb").to_str().unwrap().to_string(); + + assert_eq!(1, index.resolve("row", Some(&caller)).len()); +} + +#[test] +fn a_single_root_still_resolves() { + let root = scratch("single"); + let app = root.join("app/views"); + + write(&app.join("shared/_header.html.erb"), "
\n"); + + let templates = vec![app.join("shared/_header.html.erb").to_str().unwrap().to_string()]; + let index = PartialIndex::new(std::slice::from_ref(&app), templates); + + assert_eq!(vec!["shared/header"], index.names()); + assert_eq!(1, index.resolve("shared/header", None).len()); +} diff --git a/sig/herb/analysis/partial_index.rbs b/sig/herb/analysis/partial_index.rbs index 23fdc9d3c..9d5498d7f 100644 --- a/sig/herb/analysis/partial_index.rbs +++ b/sig/herb/analysis/partial_index.rbs @@ -5,8 +5,6 @@ module Herb class PartialIndex APPLICATION_DIRECTORY: String - attr_reader view_root: Pathname - attr_reader templates: Array[String] # : (String | Pathname, ?templates: Array[String]?) -> PartialIndex @@ -15,14 +13,22 @@ module Herb # : (String | Pathname) -> Pathname def self.resolve_view_root: (String | Pathname) -> Pathname - # : (String | Pathname, Array[String]) -> void - def initialize: (String | Pathname, Array[String]) -> void + # : (Array[String | Pathname], Array[String]) -> void + def initialize: (Array[String | Pathname], Array[String]) -> void + + # : () -> Array[Pathname] + attr_reader view_roots: untyped # : (String?) -> Array[String] def files_for: (String?) -> Array[String] + # : (String?, String?) -> Array[String] # : (String?, String?) -> Array[String] def resolve: (String?, String?) -> Array[String] + | (String?, String?) -> Array[String] + + # : (String?, String?) -> Array[String] + def candidates_for: (String?, String?) -> Array[String] # : (String, String | Pathname) -> String? def self.partial_name_for: (String, String | Pathname) -> String? diff --git a/sig/herb/analysis/partial_resolution.rbs b/sig/herb/analysis/partial_resolution.rbs index 44444f5c3..9f120a1aa 100644 --- a/sig/herb/analysis/partial_resolution.rbs +++ b/sig/herb/analysis/partial_resolution.rbs @@ -26,6 +26,18 @@ module Herb # : (String | Pathname) -> Pathname def self.view_root_for: (String | Pathname) -> Pathname + # : (String) -> String? + def self.format_of: (String) -> String? + + # : (String) -> bool + def self.has_locale?: (String) -> bool + + # : (String) -> String? + def self.variant_of: (String) -> String? + + # : (String) -> String + def self.without_template_extension: (String) -> String + # : (String) -> bool def self.template_path?: (String) -> bool @@ -44,12 +56,27 @@ module Herb # : (String, String | Pathname) -> String? def self.template_name_for: (String, String | Pathname) -> String? + # : (String, Array[String | Pathname]) -> Array[String] + def self.layout_candidates_for_roots: (String, Array[String | Pathname]) -> Array[String] + # : (String, String | Pathname) -> Array[String] def self.layout_candidates_for: (String, String | Pathname) -> Array[String] # : (String, String | Pathname) -> String? def self.partial_name_for: (String, String | Pathname) -> String? + # : (String, Array[String | Pathname]) -> [Integer, String]? + def self.relative_to_view_roots: (String, Array[String | Pathname]) -> [ Integer, String ]? + + # : (String, Array[String | Pathname]) -> String? + def self.partial_name_for_roots: (String, Array[String | Pathname]) -> String? + + # : (String, Array[String | Pathname]) -> String? + def self.template_name_for_roots: (String, Array[String | Pathname]) -> String? + + # : (String, Array[String | Pathname]) -> Integer + def self.root_index_for: (String, Array[String | Pathname]) -> Integer + # : (String, String | Pathname) -> String? private def self.relative_to_view_root: (String, String | Pathname) -> String? end diff --git a/sig/herb/analysis/project_index.rbs b/sig/herb/analysis/project_index.rbs index aca0c65cb..0a03533ab 100644 --- a/sig/herb/analysis/project_index.rbs +++ b/sig/herb/analysis/project_index.rbs @@ -21,8 +21,8 @@ module Herb # : () -> void def index_call_sites: () -> void - # : () -> Pathname? - def view_root: () -> Pathname? + # : () -> Array[Pathname]? + def view_roots: () -> Array[Pathname]? # : (String, ?String?) -> bool def handle_change: (String, ?String?) -> bool diff --git a/sig/herb/analysis/render_analyzer.rbs b/sig/herb/analysis/render_analyzer.rbs index ffa8c17b0..b6e6bbcce 100644 --- a/sig/herb/analysis/render_analyzer.rbs +++ b/sig/herb/analysis/render_analyzer.rbs @@ -117,6 +117,24 @@ module Herb def resolve_partial: (untyped partial_name, untyped source_file, untyped _partial_files, untyped view_root) -> untyped + # : (untyped) -> Array[[Hash[Symbol, untyped], Array[String]]] + def conditional_calls: (untyped) -> Array[[ Hash[Symbol, untyped], Array[String] ]] + + # : (Hash[Symbol, untyped], Hash[String, String]) -> Array[String] + def names_under: (Hash[Symbol, untyped], Hash[String, String]) -> Array[String] + + # : (String) -> bool + def missing_format?: (String) -> bool + + # : (String) -> bool + def partial_path_segment?: (String) -> bool + + # : (String) -> Array[String] + def static_branches: (String) -> Array[String] + + # : (String) -> String? + def render_name_kind: (String) -> String? + def expected_file_path: (untyped partial_name, untyped view_root) -> untyped def label: (untyped text, ?untyped width) -> untyped diff --git a/sig/herb/analysis/ruby_locals_index.rbs b/sig/herb/analysis/ruby_locals_index.rbs index 4b5a36a02..24c5cbc1a 100644 --- a/sig/herb/analysis/ruby_locals_index.rbs +++ b/sig/herb/analysis/ruby_locals_index.rbs @@ -24,7 +24,6 @@ module Herb # : (String) -> Local? def find: (String) -> Local? - # Every name the template binds, regardless of where. # : () -> Set[String] def names: () -> Set[String] diff --git a/sig/herb/analysis/ruby_locals_index/named_reference.rbs b/sig/herb/analysis/ruby_locals_index/named_reference.rbs index e63910b7c..053eb2546 100644 --- a/sig/herb/analysis/ruby_locals_index/named_reference.rbs +++ b/sig/herb/analysis/ruby_locals_index/named_reference.rbs @@ -3,8 +3,6 @@ module Herb module Analysis class RubyLocalsIndex - # A name, with where it appears in the source as a byte offset and length, - # which is how Prism reports it. class NamedReference attr_reader name: String diff --git a/sig/herb/analysis/ruby_locals_index/offset_table.rbs b/sig/herb/analysis/ruby_locals_index/offset_table.rbs index 4e2fbe794..9fc574412 100644 --- a/sig/herb/analysis/ruby_locals_index/offset_table.rbs +++ b/sig/herb/analysis/ruby_locals_index/offset_table.rbs @@ -3,8 +3,6 @@ module Herb module Analysis class RubyLocalsIndex - # Prism reports byte offsets into the whole template while the Herb AST - # reports lines and columns, so one of them has to be translated. class OffsetTable @line_starts: Array[Integer] diff --git a/sig/herb/analysis/template_dependencies.rbs b/sig/herb/analysis/template_dependencies.rbs index 0153504af..ffefc8560 100644 --- a/sig/herb/analysis/template_dependencies.rbs +++ b/sig/herb/analysis/template_dependencies.rbs @@ -60,7 +60,6 @@ module Herb def dependency_index: (untyped file_path) -> untyped # @rbs! - # KERNEL_METHODS: Array[String] KERNEL_METHODS: untyped def scan_helpers!: () -> untyped @@ -83,7 +82,6 @@ module Herb def symbol_after: (String, String) -> String? # @rbs! - # UNCOUNTABLE: Array[String] UNCOUNTABLE: untyped # : (String, String) -> String? diff --git a/test/analysis/formats_test.rb b/test/analysis/formats_test.rb new file mode 100644 index 000000000..ce4a6563c --- /dev/null +++ b/test/analysis/formats_test.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +require_relative "../test_helper" +require_relative "../../lib/herb/analysis/partial_index" + +require "tmpdir" + +module Analysis + class FormatsTest < Minitest::Spec + def write(root, relative) + path = File.join(root, relative) + + FileUtils.mkdir_p(File.dirname(path)) + File.write(path, "
\n") + + path + end + + test "reads the format out of a filename" do + assert_equal "html", Herb::Analysis::PartialResolution.format_of("app/views/posts/_row.html.erb") + assert_equal "turbo_stream", Herb::Analysis::PartialResolution.format_of("app/views/posts/_row.turbo_stream.erb") + assert_equal "html", Herb::Analysis::PartialResolution.format_of("app/views/posts/_row.html.herb") + assert_nil Herb::Analysis::PartialResolution.format_of("app/views/posts/_row.erb") + assert_nil Herb::Analysis::PartialResolution.format_of("app/views/posts/_row.herb") + assert_equal "html", Herb::Analysis::PartialResolution.format_of("app/views/posts/_row.en.html.erb") + end + + test "reads the variant out of a filename" do + assert_equal "mobile", Herb::Analysis::PartialResolution.variant_of("app/views/posts/_row.html+mobile.erb") + assert_equal "tablet", Herb::Analysis::PartialResolution.variant_of("app/views/posts/_row.html+tablet.herb") + assert_nil Herb::Analysis::PartialResolution.variant_of("app/views/posts/_row.html.erb") + assert_nil Herb::Analysis::PartialResolution.variant_of("app/views/posts/_row.erb") + end + + test "a variant keeps the format of its base template" do + assert_equal "html", Herb::Analysis::PartialResolution.format_of("app/views/posts/_row.html+mobile.erb") + assert_equal "turbo_stream", Herb::Analysis::PartialResolution.format_of("app/views/posts/_row.turbo_stream+mobile.erb") + end + + test "the plain template is preferred over a variant" do + Dir.mktmpdir do |dir| + views = File.join(dir, "app", "views") + + caller_file = write(views, "posts/index.html.erb") + variant = write(views, "posts/_row.html+mobile.erb") + plain = write(views, "posts/_row.html.erb") + + index = Herb::Analysis::PartialIndex.new([views], [caller_file, variant, plain]) + resolved = index.resolve("posts/row", caller_file) + + assert_equal plain, resolved.first + assert_includes resolved, variant + end + end + + test "a caller reaches the partial matching its own format" do + Dir.mktmpdir do |dir| + views = File.join(dir, "app", "views") + + html_caller = write(views, "posts/index.html.erb") + turbo_caller = write(views, "posts/index.turbo_stream.erb") + html_partial = write(views, "posts/_row.html.erb") + turbo_partial = write(views, "posts/_row.turbo_stream.erb") + + index = Herb::Analysis::PartialIndex.new([views], [html_caller, turbo_caller, html_partial, turbo_partial]) + + assert_equal html_partial, index.resolve("posts/row", html_caller).first + assert_equal turbo_partial, index.resolve("posts/row", turbo_caller).first + end + end + + test "a formatless partial serves any caller" do + Dir.mktmpdir do |dir| + views = File.join(dir, "app", "views") + + turbo_caller = write(views, "posts/index.turbo_stream.erb") + partial = write(views, "posts/_row.erb") + + index = Herb::Analysis::PartialIndex.new([views], [turbo_caller, partial]) + + assert_equal partial, index.resolve("posts/row", turbo_caller).first + end + end + + test "a formatless partial loses to an exact format match" do + Dir.mktmpdir do |dir| + views = File.join(dir, "app", "views") + + turbo_caller = write(views, "posts/index.turbo_stream.erb") + formatless = write(views, "posts/_row.erb") + turbo_partial = write(views, "posts/_row.turbo_stream.erb") + + index = Herb::Analysis::PartialIndex.new([views], [turbo_caller, formatless, turbo_partial]) + + assert_equal turbo_partial, index.resolve("posts/row", turbo_caller).first + end + end + + test "extension precedence still decides when no format matches" do + Dir.mktmpdir do |dir| + views = File.join(dir, "app", "views") + + turbo_caller = write(views, "posts/index.turbo_stream.erb") + html_partial = write(views, "posts/_row.html.erb") + + index = Herb::Analysis::PartialIndex.new([views], [turbo_caller, html_partial]) + + assert_equal html_partial, index.resolve("posts/row", turbo_caller).first + end + end + end +end diff --git a/test/analysis/partial_index_test.rb b/test/analysis/partial_index_test.rb index 362d9d7cd..c3ab12b52 100644 --- a/test/analysis/partial_index_test.rb +++ b/test/analysis/partial_index_test.rb @@ -25,13 +25,13 @@ def write(path, content = "
\n") test "resolves the view root to app/views when it is there" do write("app/views/posts/index.html.erb") - assert_equal File.join(@project_path, "app", "views"), Herb::Analysis::PartialIndex.build(@project_path).view_root.to_s + assert_equal [File.join(@project_path, "app", "views")], Herb::Analysis::PartialIndex.build(@project_path).view_roots.map(&:to_s) end test "falls back to the project root when there is no app/views" do write("posts/index.html.erb") - assert_equal @project_path, Herb::Analysis::PartialIndex.build(@project_path).view_root.to_s + assert_equal [@project_path], Herb::Analysis::PartialIndex.build(@project_path).view_roots.map(&:to_s) end test "maps a qualified partial name to its file" do diff --git a/test/analysis/project_index_test.rb b/test/analysis/project_index_test.rb index df4bb2963..af41ca475 100644 --- a/test/analysis/project_index_test.rb +++ b/test/analysis/project_index_test.rb @@ -135,6 +135,6 @@ def reindexed test "exposes the view root it resolved" do write("index.html.erb", "
") - assert_equal File.join(@project_path, "app", "views"), indexed.view_root.to_s + assert_equal [File.join(@project_path, "app", "views")], indexed.view_roots.map(&:to_s) end end diff --git a/test/analysis/view_roots_test.rb b/test/analysis/view_roots_test.rb new file mode 100644 index 000000000..e0bb61827 --- /dev/null +++ b/test/analysis/view_roots_test.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require_relative "../test_helper" +require_relative "../../lib/herb/analysis/partial_index" + +require "tmpdir" + +module Analysis + class ViewRootsTest < Minitest::Spec + def write(root, relative, body = "
\n") + path = File.join(root, relative) + + FileUtils.mkdir_p(File.dirname(path)) + File.write(path, body) + + path + end + + test "names a partial from a secondary view root" do + Dir.mktmpdir do |dir| + app = File.join(dir, "app", "views") + engine = File.join(dir, "engines", "billing", "app", "views") + + entry = write(app, "home/index.html.erb") + invoice = write(engine, "billing/_invoice.html.erb") + + index = Herb::Analysis::PartialIndex.new([app, engine], [entry, invoice]) + + assert_equal ["billing/invoice"], index.names + end + end + + test "an earlier view root shadows a later one" do + Dir.mktmpdir do |dir| + app = File.join(dir, "app", "views") + engine = File.join(dir, "engines", "billing", "app", "views") + + engine_invoice = write(engine, "billing/_invoice.html.erb", "
engine
\n") + app_invoice = write(app, "billing/_invoice.html.erb", "
app
\n") + + index = Herb::Analysis::PartialIndex.new([app, engine], [engine_invoice, app_invoice]) + resolved = index.resolve("billing/invoice", nil) + + assert_equal 2, resolved.size + assert_equal app_invoice, resolved.first + end + end + + test "resolves a sibling within the root that owns the caller" do + Dir.mktmpdir do |dir| + app = File.join(dir, "app", "views") + engine = File.join(dir, "engines", "billing", "app", "views") + + entry = write(engine, "billing/index.html.erb") + row = write(engine, "billing/_row.html.erb") + + index = Herb::Analysis::PartialIndex.new([app, engine], [entry, row]) + + assert_equal [row], index.resolve("row", entry) + end + end + + test "a single root still resolves" do + Dir.mktmpdir do |dir| + app = File.join(dir, "app", "views") + header = write(app, "shared/_header.html.erb") + + index = Herb::Analysis::PartialIndex.new([app], [header]) + + assert_equal ["shared/header"], index.names + assert_equal [header], index.resolve("shared/header", nil) + end + end + end +end