-
Notifications
You must be signed in to change notification settings - Fork 2
fix: resolve template directory path after esbuild bundling (refactored) #120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3f3d5cf
d5139ab
0b3b707
6ca72b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
| import { loadJsonFile } from "../loadJsonFile"; | ||
|
|
||
| vi.mock("../../sandboxes/logStep", () => ({ | ||
| logStep: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("node:fs/promises", () => ({ | ||
| default: { readFile: vi.fn() }, | ||
| })); | ||
|
|
||
| const fs = (await import("node:fs/promises")).default; | ||
| const { logStep } = await import("../../sandboxes/logStep"); | ||
|
|
||
| describe("loadJsonFile", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("returns parsed JSON on success", async () => { | ||
| vi.mocked(fs.readFile).mockResolvedValue('{"key": "value"}'); | ||
|
|
||
| const result = await loadJsonFile<Record<string, string>>("/path/to/file.json", "file.json"); | ||
|
|
||
| expect(result).toEqual({ key: "value" }); | ||
| expect(logStep).toHaveBeenCalledWith( | ||
| "loadTemplate: loaded file.json", | ||
| false, | ||
| expect.objectContaining({ sizeBytes: 16 }), | ||
| ); | ||
| }); | ||
|
|
||
| it("returns null when file does not exist", async () => { | ||
| const err = new Error("ENOENT") as NodeJS.ErrnoException; | ||
| err.code = "ENOENT"; | ||
| vi.mocked(fs.readFile).mockRejectedValue(err); | ||
|
|
||
| const result = await loadJsonFile("/path/to/missing.json", "missing.json"); | ||
|
|
||
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| it("rethrows non-ENOENT errors", async () => { | ||
| vi.mocked(fs.readFile).mockRejectedValue(new Error("EPERM")); | ||
|
|
||
| await expect(loadJsonFile("/path/to/bad.json", "bad.json")).rejects.toThrow("EPERM"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; | ||
| import path from "node:path"; | ||
| import { resolveTemplatesDir } from "../resolveTemplatesDir"; | ||
|
|
||
| vi.mock("../../sandboxes/logStep", () => ({ | ||
| logStep: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("node:fs/promises", () => ({ | ||
| default: { access: vi.fn() }, | ||
| })); | ||
|
|
||
| const fs = (await import("node:fs/promises")).default; | ||
|
|
||
| describe("resolveTemplatesDir", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("returns __dirname-relative path when it exists", async () => { | ||
| vi.mocked(fs.access).mockResolvedValueOnce(undefined); | ||
|
|
||
| const result = await resolveTemplatesDir("/app/src/content"); | ||
|
|
||
| expect(result).toBe(path.resolve("/app/src/content", "../content/templates")); | ||
| expect(fs.access).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("falls back to cwd-relative path when __dirname path missing", async () => { | ||
| const err = new Error("ENOENT") as NodeJS.ErrnoException; | ||
| err.code = "ENOENT"; | ||
| vi.mocked(fs.access).mockRejectedValueOnce(err); | ||
| vi.mocked(fs.access).mockResolvedValueOnce(undefined); | ||
|
|
||
| const result = await resolveTemplatesDir("/wrong/path"); | ||
|
|
||
| expect(result).toBe(path.resolve(process.cwd(), "src/content/templates")); | ||
| expect(fs.access).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it("rethrows permission errors instead of falling through", async () => { | ||
| const err = new Error("EACCES") as NodeJS.ErrnoException; | ||
| err.code = "EACCES"; | ||
| vi.mocked(fs.access).mockRejectedValueOnce(err); | ||
|
|
||
| await expect(resolveTemplatesDir("/app/src/content")).rejects.toThrow("EACCES"); | ||
| expect(fs.access).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("throws when no candidate directory exists", async () => { | ||
| const err = new Error("ENOENT") as NodeJS.ErrnoException; | ||
| err.code = "ENOENT"; | ||
| vi.mocked(fs.access).mockRejectedValue(err); | ||
|
|
||
| await expect(resolveTemplatesDir("/wrong/path")).rejects.toThrow( | ||
| "Templates directory not found", | ||
| ); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import fs from "node:fs/promises"; | ||
| import { logStep } from "../sandboxes/logStep"; | ||
|
|
||
| /** | ||
| * Load a JSON file. Returns null if the file doesn't exist. | ||
| * Rethrows parse errors and unexpected I/O failures so callers surface real bugs. | ||
| * | ||
| * @param filePath | ||
| * @param label | ||
| */ | ||
| export async function loadJsonFile<T>(filePath: string, label: string): Promise<T | null> { | ||
| try { | ||
| const raw = await fs.readFile(filePath, "utf-8"); | ||
| const parsed = JSON.parse(raw) as T; | ||
| logStep(`loadTemplate: loaded ${label}`, false, { | ||
| path: filePath, | ||
| sizeBytes: raw.length, | ||
| }); | ||
| return parsed; | ||
| } catch (err: unknown) { | ||
| if ( | ||
| err instanceof Error && | ||
| "code" in err && | ||
| (err as NodeJS.ErrnoException).code === "ENOENT" | ||
| ) { | ||
| return null; | ||
| } | ||
| logStep(`loadTemplate: FAILED to load ${label}`, true, { | ||
| path: filePath, | ||
| error: String(err), | ||
| }); | ||
| throw err; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import path from "node:path"; | ||
| import fs from "node:fs/promises"; | ||
| import { logStep } from "../sandboxes/logStep"; | ||
|
|
||
| /** | ||
| * Resolves the templates directory. Tries multiple strategies because | ||
| * esbuild changes __dirname at bundle time: | ||
| * 1. __dirname-relative (works locally with tsx) | ||
| * 2. process.cwd()-relative (works in Trigger.dev deployments where | ||
| * additionalFiles preserves source-root-relative paths) | ||
| * | ||
| * @param dirname - The __dirname of the calling module | ||
| */ | ||
| export async function resolveTemplatesDir(dirname: string): Promise<string> { | ||
| const candidates = [ | ||
| path.resolve(dirname, "../content/templates"), | ||
| path.resolve(process.cwd(), "src/content/templates"), | ||
| ]; | ||
|
Comment on lines
+15
to
+18
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need candidates? Shouldn't this value stay consistent?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The two candidates exist because |
||
|
|
||
| for (const dir of candidates) { | ||
| try { | ||
| await fs.access(dir); | ||
| return dir; | ||
| } catch (err: unknown) { | ||
| if (err instanceof Error && "code" in err && (err as NodeJS.ErrnoException).code === "ENOENT") { | ||
| continue; | ||
| } | ||
| throw err; | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| logStep("Template directory not found", true, { | ||
| dirname, | ||
| cwd: process.cwd(), | ||
| candidates, | ||
| }); | ||
| throw new Error( | ||
| `Templates directory not found. Tried: ${candidates.join(", ")}`, | ||
| ); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: recoupable/tasks
Length of output: 89
🏁 Script executed:
Repository: recoupable/tasks
Length of output: 7053
🏁 Script executed:
Repository: recoupable/tasks
Length of output: 89
🏁 Script executed:
Repository: recoupable/tasks
Length of output: 1213
Mirror the
ENOENT-only error handling pattern already implemented inloadJsonFile().The
fs.access()catch block at lines 39–42 masks all errors with a generic "Template directory not found" message. Permission errors (EACCES/EPERM), I/O failures, and other non-ENOENT errors should bubble up unchanged so that deployment issues surface with their actual root cause, matching the error-handling discipline already established inloadJsonFile().Proposed fix
try { await fs.access(templateDir); - } catch { - throw new Error(`Template directory not found: ${templateDir}`); + } catch (err: unknown) { + if ( + err instanceof Error && + "code" in err && + (err as NodeJS.ErrnoException).code === "ENOENT" + ) { + throw new Error(`Template directory not found: ${templateDir}`); + } + throw err; }📝 Committable suggestion
🤖 Prompt for AI Agents