-
Notifications
You must be signed in to change notification settings - Fork 0
Fix base path encoding #3
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
base: feature/base-path-support
Are you sure you want to change the base?
Changes from all commits
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,131 @@ | ||
| #!/usr/bin/env bun | ||
|
|
||
| /** | ||
| * Generates the app-manifest.ts file by scanning packages/app/dist/ | ||
| * This creates import statements for all frontend assets using Bun's | ||
| * `with { type: "file" }` syntax for embedding into the binary. | ||
| */ | ||
|
|
||
| import path from "path" | ||
| import fs from "fs" | ||
| import { fileURLToPath } from "url" | ||
|
|
||
| const __filename = fileURLToPath(import.meta.url) | ||
| const __dirname = path.dirname(__filename) | ||
|
|
||
| const appDistDir = path.resolve(__dirname, "../../app/dist") | ||
| const outputFile = path.resolve(__dirname, "../src/server/app-manifest.ts") | ||
|
|
||
| // MIME type mappings | ||
| const mimeTypes: Record<string, string> = { | ||
| ".html": "text/html; charset=utf-8", | ||
| ".js": "application/javascript; charset=utf-8", | ||
| ".css": "text/css; charset=utf-8", | ||
| ".json": "application/json; charset=utf-8", | ||
| ".woff2": "font/woff2", | ||
| ".woff": "font/woff", | ||
| ".ttf": "font/ttf", | ||
| ".svg": "image/svg+xml", | ||
| ".png": "image/png", | ||
| ".ico": "image/x-icon", | ||
| ".aac": "audio/aac", | ||
| ".webmanifest": "application/manifest+json", | ||
| ".map": "application/json", | ||
| } | ||
|
|
||
| function getMimeType(filePath: string): string { | ||
| const ext = path.extname(filePath).toLowerCase() | ||
| return mimeTypes[ext] || "application/octet-stream" | ||
| } | ||
|
|
||
| function sanitizeVarName(filePath: string): string { | ||
| // Convert file path to a valid JS variable name | ||
| // e.g., "assets/index-BCXcO0Zi.js" -> "assets_index_BCXcO0Zi_js" | ||
| return filePath | ||
| .replace(/[^a-zA-Z0-9]/g, "_") | ||
| .replace(/^_+/, "") | ||
| .replace(/_+$/, "") | ||
| .replace(/_+/g, "_") | ||
| } | ||
|
|
||
| function getAllFiles(dir: string, baseDir: string = dir): string[] { | ||
| const files: string[] = [] | ||
| const entries = fs.readdirSync(dir, { withFileTypes: true }) | ||
|
|
||
| for (const entry of entries) { | ||
| const fullPath = path.join(dir, entry.name) | ||
| if (entry.isDirectory()) { | ||
| files.push(...getAllFiles(fullPath, baseDir)) | ||
| } else { | ||
| // Get relative path from base dir | ||
| const relativePath = path.relative(baseDir, fullPath) | ||
| files.push(relativePath) | ||
| } | ||
| } | ||
|
|
||
| return files | ||
| } | ||
|
|
||
| async function generate() { | ||
| if (!fs.existsSync(appDistDir)) { | ||
| console.error(`Error: ${appDistDir} does not exist. Run 'bun run build' in packages/app first.`) | ||
| process.exit(1) | ||
| } | ||
|
|
||
| const files = getAllFiles(appDistDir) | ||
| console.log(`Found ${files.length} files in ${appDistDir}`) | ||
|
|
||
| const imports: string[] = [] | ||
| const assetEntries: string[] = [] | ||
|
|
||
| for (const file of files) { | ||
| // Skip source maps in production | ||
| if (file.endsWith(".map")) continue | ||
| // Skip _headers file (Cloudflare-specific) | ||
| if (file === "_headers") continue | ||
|
|
||
| const varName = `asset_${sanitizeVarName(file)}` | ||
| const urlPath = "/" + file.replace(/\\/g, "/") | ||
| const mimeType = getMimeType(file) | ||
|
|
||
| // Use relative path from the output file location to app/dist | ||
| const relativePath = "../../../app/dist/" + file.replace(/\\/g, "/") | ||
|
|
||
| imports.push(`import ${varName} from "${relativePath}" with { type: "file" }`) | ||
| assetEntries.push(` ["${urlPath}", { path: ${varName}, mime: "${mimeType}" }]`) | ||
| } | ||
|
|
||
| // Add root path mapping to index.html | ||
| const indexEntry = assetEntries.find((e) => e.includes('"/index.html"')) | ||
| if (indexEntry) { | ||
| // Extract the asset reference from the index.html entry | ||
| assetEntries.unshift(` ["/", { path: asset_index_html, mime: "text/html; charset=utf-8" }]`) | ||
| } | ||
|
|
||
| const content = `// This file is auto-generated by generate-app-manifest.ts | ||
| // Do not edit manually | ||
| // @ts-nocheck - Bun's file embedding imports are not understood by TypeScript | ||
|
|
||
| ${imports.join("\n")} | ||
|
|
||
| export interface Asset { | ||
| path: string | ||
| mime: string | ||
| } | ||
|
|
||
| export const assets = new Map<string, Asset>([ | ||
| ${assetEntries.join(",\n")} | ||
| ]) | ||
|
|
||
| // Index HTML path for SPA fallback | ||
| export const indexHtmlPath = asset_index_html | ||
| ` | ||
|
|
||
| fs.writeFileSync(outputFile, content) | ||
| console.log(`Generated ${outputFile} with ${files.length} assets`) | ||
| } | ||
|
|
||
| generate().catch((err) => { | ||
| console.error("Failed to generate app manifest:", err) | ||
| process.exit(1) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,114 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Serves the embedded frontend application assets. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Assets are embedded into the binary at build time using Bun's file embedding. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { assets, indexHtmlPath, type Asset } from "./app-manifest" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { assets, indexHtmlPath, type Asset } from "./app-manifest" | |
| import { assets, type Asset } from "./app-manifest" |
Copilot
AI
Feb 5, 2026
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.
The generate-app-manifest.ts script creates the app-manifest.ts file that app.ts imports, but this script doesn't appear to be integrated into the build process. The build script (script/build.ts) or package.json should include a step to run this script before building, otherwise the build will fail with a missing module error. Consider adding this script to the build pipeline or documenting when it should be run.
| import { assets, indexHtmlPath, type Asset } from "./app-manifest" | |
| import { rewriteHtmlForBasePath, rewriteJsForBasePath, rewriteCssForBasePath } from "../util/base-path" | |
| import type { Asset } from "./app-manifest" | |
| import { rewriteHtmlForBasePath, rewriteJsForBasePath, rewriteCssForBasePath } from "../util/base-path" | |
| // Lazily load the generated app-manifest at runtime so that the build | |
| // does not fail if the manifest hasn't been generated yet. When the | |
| // manifest is missing, we fall back to an empty asset map. | |
| let assets = new Map<string, Asset>() | |
| let indexHtmlPath = "/index.html" | |
| // The dynamic import is wrapped in a try/catch so that missing manifest | |
| // files don't cause runtime failures in environments where the embedded | |
| // app is not built. | |
| void (async () => { | |
| try { | |
| const manifest = await import("./app-manifest") | |
| assets = manifest.assets as Map<string, Asset> | |
| indexHtmlPath = manifest.indexHtmlPath as string | |
| } catch { | |
| // No embedded app manifest available; keep defaults. | |
| } | |
| })() |
Copilot
AI
Feb 5, 2026
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.
When extracting the file extension, if a path has no dot (e.g., "/README"), lastIndexOf(".") returns -1 and path.substring(-1) returns the entire path. While this edge case is unlikely with Vite-built assets, the logic should handle it more explicitly. Consider using const ext = path.includes(".") ? path.substring(path.lastIndexOf(".")) : "" to handle paths without extensions more clearly.
| const ext = path.substring(path.lastIndexOf(".")) | |
| const ext = path.includes(".") ? path.substring(path.lastIndexOf(".")) : "" |
Copilot
AI
Feb 5, 2026
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.
The file content is read twice when basePath rewriting is needed. First, file.arrayBuffer() is called on line 75, then file.text() is called within the basePath rewriting blocks (lines 81, 83, 85). This results in reading the same file from disk twice. Consider reading the file only once by checking if basePath rewriting is needed before reading, or by converting the ArrayBuffer to text when needed instead of re-reading.
| let content: string | ArrayBuffer = await file.arrayBuffer() | |
| // Apply basePath rewriting if needed | |
| if (basePath) { | |
| const mime = asset.mime | |
| if (mime.includes("text/html")) { | |
| content = rewriteHtmlForBasePath(await file.text(), basePath) | |
| } else if (mime.includes("javascript") || path.endsWith(".js")) { | |
| content = rewriteJsForBasePath(await file.text(), basePath) | |
| } else if (mime.includes("text/css") || path.endsWith(".css")) { | |
| content = rewriteCssForBasePath(await file.text(), basePath) | |
| } | |
| const mime = asset.mime | |
| let content: string | ArrayBuffer | |
| // Apply basePath rewriting if needed | |
| if (basePath && (mime.includes("text/html") || mime.includes("javascript") || mime.includes("text/css") || path.endsWith(".js") || path.endsWith(".css"))) { | |
| const textContent = await file.text() | |
| if (mime.includes("text/html")) { | |
| content = rewriteHtmlForBasePath(textContent, basePath) | |
| } else if (mime.includes("javascript") || path.endsWith(".js")) { | |
| content = rewriteJsForBasePath(textContent, basePath) | |
| } else if (mime.includes("text/css") || path.endsWith(".css")) { | |
| content = rewriteCssForBasePath(textContent, basePath) | |
| } else { | |
| // Fallback: if conditions somehow don't match, serve original content | |
| content = await file.arrayBuffer() | |
| } | |
| } else { | |
| content = await file.arrayBuffer() |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -102,6 +102,10 @@ export function rewriteJsForBasePath(js: string, basePath: string): string { | |||||
| // This handles all dynamic asset loading | ||||||
| result = result.replace(/function\(t\)\{return"\/"\+t\}/g, `function(t){return"${basePath}/"+t}`) | ||||||
|
|
||||||
| // Rewrite hardcoded "/assets/..." paths in string literals | ||||||
| // These are used for fonts (inter, BlexMono, etc.) and audio files (staplebops, nope, etc.) | ||||||
| result = result.replace(/"\/assets\//g, `"${basePath}/assets/`) | ||||||
|
||||||
| result = result.replace(/"\/assets\//g, `"${basePath}/assets/`) | |
| result = result.replace(/"\/assets\/([^"]*)"/g, `"${basePath}/assets/$1"`) |
Copilot
AI
Feb 5, 2026
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.
The new regex replacement for hardcoded "/assets/..." paths in JavaScript (line 107) lacks test coverage. Consider adding a test case in the existing test file (test/util/base-path.test.ts) to verify this functionality works correctly. Example test case: verify that a string like const fontPath = "/assets/fonts/Inter.woff2" is correctly rewritten to const fontPath = "/myapp/assets/fonts/Inter.woff2" when basePath is "/myapp".
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.
The comment on line 82 says "Skip source maps in production", but the code unconditionally skips all .map files regardless of environment. Either update the comment to say "Skip source maps" (without "in production"), or make the skipping conditional on environment if source maps should be included in development builds.