diff --git a/.changeset/route-entry-names-relative.md b/.changeset/route-entry-names-relative.md new file mode 100644 index 00000000..2504c891 --- /dev/null +++ b/.changeset/route-entry-names-relative.md @@ -0,0 +1,19 @@ +--- +'rsbuild-plugin-react-router': patch +--- + +Derive route entry names from the route file instead of the route id. A route +table built with `relative()` resolves route files to absolute paths, so React +Router relativizes `file` but leaves `id` absolute, and the plugin used that +opaque id as an rspack entry name. Split route module chunks were therefore +emitted into a directory tree mirroring the developer's checkout +(`static/js/Users//.../customers-client-loader.js`) and the browser +manifest published those paths, leaking `$HOME` into production assets and +making builds unreproducible across machines and CI. Entry names are now +app-relative for both the route entry and its chunks, so a chunk lands beside +its route, the `"/static/js//..."` double slash is gone, and an entry can no +longer escape the JS output directory or carry a Windows drive prefix. Route +ids are untouched: they remain the runtime contract behind +`useRouteLoaderData(id)` and `matches[].id`. Note that route chunks for routes +declared with an explicit `id` are renamed accordingly, which changes those +asset URLs. diff --git a/src/classic-mode.ts b/src/classic-mode.ts index 47ed8509..bd96ccf4 100644 --- a/src/classic-mode.ts +++ b/src/classic-mode.ts @@ -25,6 +25,7 @@ import { import { getRouteChunkEntryName, getRouteChunkModuleId, + getRouteEntryBaseName, routeChunkExportNames, } from './route-chunks.js'; import type { Config } from './react-router-config.js'; @@ -144,7 +145,7 @@ export const createClassicWebRouteEntries = ({ const manifestChunkNames = new Set(['entry.client']); const webRouteEntries = Object.values(routes).reduce( (acc, route) => { - const entryName = route.file.slice(0, route.file.lastIndexOf('.')); + const entryName = getRouteEntryBaseName(route, appDirectory); const routeFilePath = resolve(appDirectory, route.file); manifestChunkNames.add(entryName); acc[entryName] = { @@ -166,7 +167,11 @@ export const createClassicWebRouteEntries = ({ if (!source.includes(exportName)) { continue; } - const chunkEntryName = getRouteChunkEntryName(route.id, exportName); + const chunkEntryName = getRouteChunkEntryName( + route, + exportName, + appDirectory + ); manifestChunkNames.add(chunkEntryName); acc[chunkEntryName] = { import: getRouteChunkModuleId(routeFilePath, exportName), diff --git a/src/manifest.ts b/src/manifest.ts index 3d6cd61f..7bae6810 100644 --- a/src/manifest.ts +++ b/src/manifest.ts @@ -13,6 +13,7 @@ import { createEmptyRouteChunkByExportName, detectRouteChunksIfEnabled, getRouteChunkEntryName, + getRouteEntryBaseName, routeChunkExportNames, validateRouteChunks, type RouteChunkCache, @@ -425,23 +426,19 @@ const getManifestVersion = ( .slice(0, 8); }; -const getRouteEntryName = (route: Route): string => { - const extensionIndex = route.file.lastIndexOf('.'); - return extensionIndex >= 0 ? route.file.slice(0, extensionIndex) : route.file; -}; - export const getReactRouterManifestChunkNames = ( routes: Record, + appDirectory: string, splitRouteModules: boolean | 'enforce' = false ): Set => { const chunkNames = new Set(['entry.client']); for (const route of Object.values(routes)) { - chunkNames.add(getRouteEntryName(route)); + chunkNames.add(getRouteEntryBaseName(route, appDirectory)); if (!splitRouteModules || route.id === 'root') { continue; } for (const exportName of routeChunkExportNames) { - chunkNames.add(getRouteChunkEntryName(route.id, exportName)); + chunkNames.add(getRouteChunkEntryName(route, exportName, appDirectory)); } } return chunkNames; @@ -449,6 +446,7 @@ export const getReactRouterManifestChunkNames = ( const createRouteManifestItem = ({ route, + appDirectory, assetPrefix, jsAssets, routeAnalysis, @@ -456,6 +454,7 @@ const createRouteManifestItem = ({ getCssAssetsForChunk, }: { route: Route; + appDirectory: string; assetPrefix: string; jsAssets: string[]; routeAnalysis: RouteManifestAnalysis; @@ -465,13 +464,17 @@ const createRouteManifestItem = ({ const routeChunkMap = routeAnalysis.hasRouteChunkByExportName; const chunkModulePath = (exportName: RouteChunkExportName) => routeChunkMap?.[exportName] - ? getModulePathForChunk(getRouteChunkEntryName(route.id, exportName)) + ? getModulePathForChunk( + getRouteChunkEntryName(route, exportName, appDirectory) + ) : undefined; const cssAssets = [ ...routeAnalysis.cssAssets, ...routeChunkExportNames.flatMap(exportName => routeChunkMap?.[exportName] - ? getCssAssetsForChunk(getRouteChunkEntryName(route.id, exportName)) + ? getCssAssetsForChunk( + getRouteChunkEntryName(route, exportName, appDirectory) + ) : [] ), ]; @@ -540,7 +543,7 @@ function generateReactRouterManifestForDevEffect( Object.entries(routes), ([key, route]) => Effect.gen(function* () { - const routeEntryName = getRouteEntryName(route); + const routeEntryName = getRouteEntryBaseName(route, context); const { js: jsAssets, css: discoveredCssAssets } = getAssetsForChunk(routeEntryName); const routeFilePath = resolve(context, route.file); @@ -571,6 +574,7 @@ function generateReactRouterManifestForDevEffect( key, createRouteManifestItem({ route, + appDirectory: context, assetPrefix, jsAssets, routeAnalysis, diff --git a/src/modify-browser-manifest.ts b/src/modify-browser-manifest.ts index f85026ff..f8ecc75f 100644 --- a/src/modify-browser-manifest.ts +++ b/src/modify-browser-manifest.ts @@ -147,6 +147,7 @@ export function registerModifyBrowserManifestAssets( options?.manifestChunkNames ?? getReactRouterManifestChunkNames( routes, + appDirectory, routeChunkOptions?.splitRouteModules ); const isBuild = Boolean(routeChunkOptions?.isBuild); diff --git a/src/route-chunks.ts b/src/route-chunks.ts index fc0d140c..7d5473db 100644 --- a/src/route-chunks.ts +++ b/src/route-chunks.ts @@ -860,6 +860,19 @@ const normalizeRelativeFilePath = (file: string, appDirectory: string) => { return normalize(relativePath).split('?')[0]; }; +// An rspack entry name is written out as a path under `output.distPath.js`, so it +// is a filename, not an identifier. Keep it a safe, app-relative POSIX path: the +// developer's absolute checkout path must never reach `dist/` (nor the browser +// manifest), and an entry must never escape the JS output directory. `pathe` +// already normalizes separators, so only drive prefixes and `..` need handling. +const toSafeEntryPath = (relativePath: string): string => + relativePath + .replace(/^[A-Za-z]:/, '') + .split('/') + .filter(segment => segment !== '' && segment !== '.') + .map(segment => (segment === '..' ? '__' : segment)) + .join('/'); + const isRootRouteModuleId = (config: RouteChunkConfig, id: string) => normalizeRelativeFilePath(id, config.appDirectory) === config.rootRouteFile; @@ -967,7 +980,20 @@ export const validateRouteChunks: (args: { ); }; +export const getRouteEntryBaseName = ( + route: { file: string }, + appDirectory: string +): string => + toSafeEntryPath(normalizeRelativeFilePath(route.file, appDirectory)).replace( + /\.[^/.]+$/, + '' + ); + export const getRouteChunkEntryName = ( - routeId: string, - chunkName: RouteChunkExportName -) => `${routeId}-${routeChunkEntrySuffix[chunkName]}`; + route: { file: string }, + chunkName: RouteChunkExportName, + appDirectory: string +): string => + `${getRouteEntryBaseName(route, appDirectory)}-${ + routeChunkEntrySuffix[chunkName] + }`; diff --git a/tests/classic-web-route-entries.test.ts b/tests/classic-web-route-entries.test.ts new file mode 100644 index 00000000..9d77ec5a --- /dev/null +++ b/tests/classic-web-route-entries.test.ts @@ -0,0 +1,99 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { describe, expect, it } from '@rstest/core'; +import { createClassicWebRouteEntries } from '../src/classic-mode'; +import type { Route } from '../src/types'; + +const ROUTE_SOURCE = `export async function clientLoader() { return {}; } + export default function X() { return null; }`; + +/** + * Lays out a project whose route module lives *outside* `app/`, which is what a + * shared route package or a `relative()` route table produces. + */ +const createAppWithOutsideRoute = () => { + const root = mkdtempSync(join(tmpdir(), 'rr-entries-')); + const appDir = join(root, 'app'); + const sharedDir = join(root, 'shared'); + mkdirSync(appDir, { recursive: true }); + mkdirSync(sharedDir, { recursive: true }); + + writeFileSync( + join(appDir, 'root.tsx'), + `export default function Root() { return null; }` + ); + writeFileSync(join(sharedDir, 'x.tsx'), ROUTE_SOURCE); + + return { root, appDir }; +}; + +describe('createClassicWebRouteEntries', () => { + it('never emits an entry name that escapes the JS output directory', () => { + const { root, appDir } = createAppWithOutsideRoute(); + const routes: Record = { + root: { id: 'root', file: 'root.tsx', path: '' }, + 'shared/x': { + id: 'shared/x', + parentId: 'root', + file: '../shared/x.tsx', + path: 'x', + }, + }; + + try { + const { webRouteEntries } = createClassicWebRouteEntries({ + appDirectory: appDir, + isBuild: true, + routes, + splitRouteModules: true, + }); + + expect(Object.keys(webRouteEntries).sort()).toEqual([ + '__/shared/x', + '__/shared/x-client-loader', + 'root', + ]); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + + it('shares one entry between routes that point at the same file', () => { + // React Router requires explicit ids when two routes reuse a file. Both + // routes import the very same module, and a route chunk is a pure function + // of (file, export) — so one entry serves both instead of emitting two + // byte-identical chunks. + const root = mkdtempSync(join(tmpdir(), 'rr-entries-')); + const appDir = join(root, 'app'); + mkdirSync(join(appDir, 'routes'), { recursive: true }); + writeFileSync( + join(appDir, 'root.tsx'), + `export default function Root() { return null; }` + ); + writeFileSync(join(appDir, 'routes', 'shared.tsx'), ROUTE_SOURCE); + + const routes: Record = { + root: { id: 'root', file: 'root.tsx', path: '' }, + a: { id: 'a', parentId: 'root', file: 'routes/shared.tsx', path: 'a' }, + b: { id: 'b', parentId: 'root', file: 'routes/shared.tsx', path: 'b' }, + }; + + try { + const { webRouteEntries } = createClassicWebRouteEntries({ + appDirectory: appDir, + isBuild: true, + routes, + splitRouteModules: true, + }); + + expect(Object.keys(webRouteEntries).sort()).toEqual([ + 'root', + 'routes/shared', + 'routes/shared-client-loader', + ]); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); +}); diff --git a/tests/manifest-split-route-modules.test.ts b/tests/manifest-split-route-modules.test.ts index 2f77f9bb..7df76196 100644 --- a/tests/manifest-split-route-modules.test.ts +++ b/tests/manifest-split-route-modules.test.ts @@ -5,6 +5,7 @@ import { describe, expect, it } from '@rstest/core'; import { getReactRouterManifestForDev } from '../src/manifest'; import { getRouteChunkEntryName, + getRouteEntryBaseName, routeChunkExportNames, type RouteChunkExportName, } from '../src/route-chunks'; @@ -63,15 +64,17 @@ const routes = { }, }; -const createClientStats = (routeId = 'routes/clients') => { +const clientsRoute = { file: 'routes/clients.tsx' }; + +const createClientStats = (appDirectory: string, route = clientsRoute) => { + const entryName = getRouteEntryBaseName(route, appDirectory); const assetsByChunkName: Record = { 'entry.client': ['static/js/entry.client.js'], - [routeId]: [`static/js/${routeId}.js`], + [entryName]: [`static/js/${entryName}.js`], }; for (const exportName of routeChunkExportNames) { - assetsByChunkName[getRouteChunkEntryName(routeId, exportName)] = [ - `static/js/${getRouteChunkEntryName(routeId, exportName)}.js`, - ]; + const chunkName = getRouteChunkEntryName(route, exportName, appDirectory); + assetsByChunkName[chunkName] = [`static/js/${chunkName}.js`]; } return { assetsByChunkName }; }; @@ -81,12 +84,19 @@ const getManifest = async ( splitRouteModules: boolean | 'enforce', isBuild = true ) => - getReactRouterManifestForDev(routes, {}, createClientStats(), appDir, '/', { - splitRouteModules, - rootRouteFile: 'root.tsx', - isBuild, - cache: new Map(), - }); + getReactRouterManifestForDev( + routes, + {}, + createClientStats(appDir), + appDir, + '/', + { + splitRouteModules, + rootRouteFile: 'root.tsx', + isBuild, + cache: new Map(), + } + ); describe('manifest split route modules', () => { it.each(routeChunkExportNames)( @@ -101,7 +111,7 @@ describe('manifest split route modules', () => { const field = moduleFieldByExportName[exportName]; expect(manifest.routes['routes/clients'][field]).toBe( - `/static/js/${getRouteChunkEntryName('routes/clients', exportName)}.js` + `/static/js/${getRouteChunkEntryName(clientsRoute, exportName, appDir)}.js` ); } finally { rmSync(root, { recursive: true, force: true }); @@ -118,9 +128,9 @@ describe('manifest split route modules', () => { export default function Clients() { return null; } `); writeFileSync(join(appDir, 'routes/clients.module.css'), '.root {}'); - const clientStats = createClientStats(); + const clientStats = createClientStats(appDir); clientStats.assetsByChunkName[ - getRouteChunkEntryName('routes/clients', 'clientLoader') + getRouteChunkEntryName(clientsRoute, 'clientLoader', appDir) ]?.push('static/css/routes/clients-client-loader.css'); try { @@ -211,6 +221,105 @@ describe('manifest split route modules', () => { } }); + it('keeps the app directory out of asset URLs when route ids are absolute', async () => { + // `relative(import.meta.dirname)` resolves route files to absolute paths, so + // React Router relativizes `file` but derives an *absolute* route id. The id + // is an opaque runtime identifier and must never become a filename. + const { root, appDir } = createTempApp(); + const absoluteId = `${appDir}/routes/clients`; + const routesWithAbsoluteId = { + root: { id: 'root', file: 'root.tsx', path: '' }, + [absoluteId]: { + id: absoluteId, + parentId: 'root', + file: 'routes/clients.tsx', + path: 'clients', + }, + }; + + try { + const manifest = await getReactRouterManifestForDev( + routesWithAbsoluteId, + {}, + createClientStats(appDir), + appDir, + '/', + { + splitRouteModules: true, + rootRouteFile: 'root.tsx', + isBuild: true, + cache: new Map(), + } + ); + const route = manifest.routes[absoluteId]; + + expect(route.clientLoaderModule).toBe( + '/static/js/routes/clients-client-loader.js' + ); + expect(route.clientActionModule).toBe( + '/static/js/routes/clients-client-action.js' + ); + + // The route id stays absolute — that is the runtime contract, and changing + // it would break `useRouteLoaderData(id)` and `matches[].id`. + expect(route.id).toBe(absoluteId); + + // No URL may carry the developer's checkout path, and none may contain the + // `static/js//` double slash that an entry name starting with `/` produced. + const urls = [ + route.module, + route.clientActionModule, + route.clientLoaderModule, + route.clientMiddlewareModule, + route.hydrateFallbackModule, + ...route.imports, + ...route.css, + ].filter((url): url is string => typeof url === 'string'); + + expect(urls.filter(url => url.includes(appDir))).toEqual([]); + expect(urls.filter(url => url.includes('static/js//'))).toEqual([]); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + + it('resolves routes sharing a file to the same chunk asset', async () => { + // React Router requires explicit ids when two routes reuse a file. Both + // resolve to one chunk, because a route chunk is a pure function of + // (file, export) — previously they produced two byte-identical chunks. + const { root, appDir } = createTempApp(); + const sharedRoutes = { + root: { id: 'root', file: 'root.tsx', path: '' }, + a: { id: 'a', parentId: 'root', file: 'routes/clients.tsx', path: 'a' }, + b: { id: 'b', parentId: 'root', file: 'routes/clients.tsx', path: 'b' }, + }; + + try { + const manifest = await getReactRouterManifestForDev( + sharedRoutes, + {}, + createClientStats(appDir), + appDir, + '/', + { + splitRouteModules: true, + rootRouteFile: 'root.tsx', + isBuild: true, + cache: new Map(), + } + ); + + expect(manifest.routes.a.clientLoaderModule).toBe( + '/static/js/routes/clients-client-loader.js' + ); + expect(manifest.routes.b.clientLoaderModule).toBe( + manifest.routes.a.clientLoaderModule + ); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + it('does not add route chunk module fields for the root route', async () => { const { root, appDir } = createTempApp( `export default function Clients() { return null; }`, diff --git a/tests/manifest.test.ts b/tests/manifest.test.ts index 1ccc3935..f95f88bf 100644 --- a/tests/manifest.test.ts +++ b/tests/manifest.test.ts @@ -223,11 +223,11 @@ describe('manifest', () => { }); it('collects only manifest-readable chunk names', () => { - expect(Array.from(getReactRouterManifestChunkNames(routes, false))).toEqual( + expect(Array.from(getReactRouterManifestChunkNames(routes, '/app', false))).toEqual( ['entry.client', 'root', 'routes/page'] ); - expect(getReactRouterManifestChunkNames(routes, true)).toEqual( + expect(getReactRouterManifestChunkNames(routes, '/app', true)).toEqual( new Set([ 'entry.client', 'root', diff --git a/tests/react-router-framework/integration/route-entry-names-test.ts b/tests/react-router-framework/integration/route-entry-names-test.ts new file mode 100644 index 00000000..e5ad943f --- /dev/null +++ b/tests/react-router-framework/integration/route-entry-names-test.ts @@ -0,0 +1,94 @@ +import { readdirSync, readFileSync } from "node:fs"; +import path from "node:path"; +import { test, expect } from "@playwright/test"; + +import { createProject, build, reactRouterConfig } from "./helpers/rsbuild.js"; + +const js = String.raw; + +// `fs.globSync("**/*")` silently stops short of deeply nested files, which is +// precisely where a leaked absolute path lands, so walk the tree instead. +const listEmittedFiles = (cwd: string, dir: string) => + readdirSync(path.join(cwd, dir), { recursive: true }) + .map(String) + .filter((file) => file.endsWith(".js")); + +// A route table built with `relative()` resolves route files to absolute paths, +// so React Router relativizes `file` but derives an *absolute* route id. The id +// is an opaque runtime identifier; when it is used as an rspack entry name the +// chunk is written into a directory tree mirroring the developer's checkout and +// that path is published in the browser manifest. +test.describe("Route entry names", () => { + test("keeps the app directory out of emitted assets and the browser manifest", async () => { + let cwd = await createProject({ + // SPA + framework mode, where this was first observed. SSR shares the very + // same route entry construction. + "react-router.config.ts": reactRouterConfig({ ssr: false }), + "app/routes.ts": js` + import type { RouteConfig } from "@react-router/dev/routes"; + import { relative } from "@react-router/dev/routes"; + + const { route } = relative(import.meta.dirname); + + export default [ + route("customers", "routes/customers.tsx"), + ] satisfies RouteConfig; + `, + "app/routes/customers.tsx": js` + export async function clientLoader() { + return { name: "Ada" }; + } + + export default function Customers() { + return

Customers

; + } + `, + }); + + let { status, stderr } = build({ cwd }); + expect(stderr.toString()).toBe(""); + expect(status).toBe(0); + + let emitted = listEmittedFiles(cwd, "build/client"); + + // An absolute path leaks either verbatim or with its leading separator + // swallowed by the `static/js/` join, so check for both shapes. + let appDirectory = path.join(cwd, "app"); + let leaks = [cwd, appDirectory].flatMap((absolute) => [ + absolute, + absolute.replace(/^[/\\]+/, ""), + ]); + let leaking = (text: string) => leaks.filter((leak) => text.includes(leak)); + + expect(emitted.filter((file) => leaking(file).length > 0)).toEqual([]); + + // The route chunk must be a sibling of the route entry itself. + expect(emitted).toContain( + path.join("static/js/routes/customers-client-loader.js"), + ); + + let manifestFile = emitted.find((file) => + /static[/\\]js[/\\]manifest-[^/\\]+\.js$/.test(file), + ); + expect(manifestFile).toBeDefined(); + + let manifestSource = readFileSync( + path.join(cwd, "build/client", manifestFile!), + "utf8", + ); + let assetUrls = (manifestSource.match(/['"]\/static\/[^'"]*['"]/g) ?? []).map( + (quoted) => quoted.slice(1, -1), + ); + // Guard against a vacuous pass if the manifest format ever changes. + expect(assetUrls.length).toBeGreaterThan(0); + + expect(assetUrls.filter((url) => leaking(url).length > 0)).toEqual([]); + // An entry name starting with `/` produced a `/static/js//...` double slash. + expect(assetUrls.filter((url) => url.includes("//"))).toEqual([]); + expect(assetUrls).toContain("/static/js/routes/customers-client-loader.js"); + + // The route id itself stays absolute: it is the runtime contract behind + // `useRouteLoaderData(id)` and `matches[].id`, and must not be sanitized. + expect(manifestSource).toContain(path.join(appDirectory, "routes/customers")); + }); +}); diff --git a/tests/route-chunks.test.ts b/tests/route-chunks.test.ts index 87cc64de..eaed595f 100644 --- a/tests/route-chunks.test.ts +++ b/tests/route-chunks.test.ts @@ -7,6 +7,7 @@ import { getRouteChunkIfEnabled, getRouteChunkModuleId, getRouteChunkNameFromModuleId, + getRouteEntryBaseName, isRouteChunkModuleId, routeChunkExportNames, type RouteChunkConfig, @@ -674,9 +675,13 @@ describe('route chunks', () => { expect( getRouteChunkNameFromModuleId('/app/routes/r.tsx?route-chunk=not-valid') ).toBeNull(); - expect(getRouteChunkEntryName('routes/clients', 'clientAction')).toBe( - 'routes/clients-client-action' - ); + expect( + getRouteChunkEntryName( + { file: 'routes/clients.tsx' }, + 'clientAction', + '/app' + ) + ).toBe('routes/clients-client-action'); }); }); @@ -821,3 +826,44 @@ describe('route chunks', () => { }); }); }); + +describe('route entry names', () => { + it('derives the entry base name from the route file', () => { + expect( + getRouteEntryBaseName( + { file: 'domains/customers/routes/customers.tsx' }, + '/Users/dev/proj/app' + ) + ).toBe('domains/customers/routes/customers'); + }); + + it('keeps the entry name inside the JS output directory', () => { + // `relative()` from `@react-router/dev/routes` hands us absolute files. + expect( + getRouteEntryBaseName( + { file: '/Users/dev/proj/app/routes/a.tsx' }, + '/Users/dev/proj/app' + ) + ).toBe('routes/a'); + + // A route outside `appDirectory` must not escape `static/js`. + expect( + getRouteEntryBaseName({ file: '../shared/x.tsx' }, '/Users/dev/proj/app') + ).toBe('__/shared/x'); + + // A Windows drive letter is not a legal filename character. + expect( + getRouteEntryBaseName({ file: 'C:/other/x.tsx' }, 'D:/proj/app') + ).toBe('other/x'); + }); + + it('names a route chunk as a sibling of the route entry', () => { + expect( + getRouteChunkEntryName( + { file: 'routes/clients.tsx' }, + 'clientAction', + '/Users/dev/proj/app' + ) + ).toBe('routes/clients-client-action'); + }); +});