Skip to content

Commit

Permalink
fix(remix-dev): relativize route modules to make builds deterministic
Browse files Browse the repository at this point in the history
Fixes remix-run#2024

If virtual modules have non-deterministic paths or content (e.g. due to
importing from other absolute paths), the input is technically different,
and deterministic build output is not guaranteed.

Depending on how you build/deploy (e.g. if you need to build and deploy
your server separately from your browser build), this can result in a broken
app, since the server and browser manifests may differ (i.e. due to different
fingerprints). By using relative paths for route modules, we can ensure the
same result no matter the absolute path.

Possibly worth pointing out that this fix also affects file path comments in
the server build, e.g. you'll now see stuff like:
 // app/root.tsx
instead of:
 // /absolute/path/on/the/build/machine/to/app/root.tsx

Testing notes:
 1. Added integration test
 2. Verified manually, i.e.
    1. Create two remix projects (via npx create-remix@latest)
    2. `npm run build` them both
    3. `diff -r project1/build project2/build` has no differences
    4. `diff -r project1/public/build project2/public/build` has no differences
    5. `dev` and `start` still work as per usual
  • Loading branch information
jenseng authored and MichaelDeBoey committed Jun 3, 2022
1 parent 5f979ca commit 7a2188d
Show file tree
Hide file tree
Showing 6 changed files with 45 additions and 19 deletions.
28 changes: 28 additions & 0 deletions integration/deterministic-build-output-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { test, expect } from "@playwright/test";
import globby from "globby";
import fs from "fs";
import path from "path";

import { createFixtureProject, js } from "./helpers/create-fixture";

test("builds deterministically under different paths", async () => {
let dir1 = await createFixtureProject();
let dir2 = await createFixtureProject();

expect(dir1).not.toEqual(dir2);

let files1 = await globby(["build/index.js", "public/build/**/*.js"], {
cwd: dir1,
});
let files2 = await globby(["build/index.js", "public/build/**/*.js"], {
cwd: dir2,
});

expect(files1.length).toBeGreaterThan(0);
expect(files1).toEqual(files2);
files1.forEach((file, i) => {
expect(fs.readFileSync(path.join(dir1, file))).toEqual(
fs.readFileSync(path.join(dir2, files2[i]))
);
});
});
8 changes: 5 additions & 3 deletions integration/helpers/create-fixture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const TMP_DIR = path.join(process.cwd(), ".tmp", "integration");
interface FixtureInit {
buildStdio?: Writable;
sourcemap?: boolean;
files: { [filename: string]: string };
files?: { [filename: string]: string };
template?: "cf-template" | "deno-template" | "node-template";
setup?: "node" | "cloudflare";
}
Expand Down Expand Up @@ -143,7 +143,9 @@ export async function createAppFixture(fixture: Fixture) {
}

////////////////////////////////////////////////////////////////////////////////
export async function createFixtureProject(init: FixtureInit): Promise<string> {
export async function createFixtureProject(
init: FixtureInit = {}
): Promise<string> {
let template = init.template ?? "node-template";
let integrationTemplateDir = path.join(__dirname, template);
let projectName = `remix-${template}-${Math.random().toString(32).slice(2)}`;
Expand Down Expand Up @@ -186,7 +188,7 @@ function build(projectDir: string, buildStdio?: Writable, sourcemap?: boolean) {

async function writeTestFiles(init: FixtureInit, dir: string) {
await Promise.all(
Object.keys(init.files).map(async (filename) => {
Object.keys(init.files ?? {}).map(async (filename) => {
let filePath = path.join(dir, filename);
await fse.ensureDir(path.dirname(filePath));
await fse.writeFile(filePath, stripIndent(init.files[filename]));
Expand Down
3 changes: 1 addition & 2 deletions packages/remix-dev/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -343,8 +343,7 @@ async function createBrowserBuild(
// All route entry points are virtual modules that will be loaded by the
// browserEntryPointsPlugin. This allows us to tree-shake server-only code
// that we don't want to run in the browser (i.e. action & loader).
entryPoints[id] =
path.resolve(config.appDirectory, config.routes[id].file) + "?browser";
entryPoints[id] = config.routes[id].file + "?browser";
}

let plugins = [
Expand Down
11 changes: 6 additions & 5 deletions packages/remix-dev/compiler/assets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export async function createAssetsManifest(
let routesByFile: Map<string, Route> = Object.keys(config.routes).reduce(
(map, key) => {
let route = config.routes[key];
map.set(path.resolve(config.appDirectory, route.file), route);
map.set(route.file, route);
return map;
},
new Map()
Expand All @@ -72,16 +72,17 @@ export async function createAssetsManifest(
let output = metafile.outputs[key];
if (!output.entryPoint) continue;

let entryPointFile = path.resolve(
output.entryPoint.replace(/(^browser-route-module:|\?browser$)/g, "")
);
if (entryPointFile === entryClientFile) {
if (path.resolve(output.entryPoint) === entryClientFile) {
entry = {
module: resolveUrl(key),
imports: resolveImports(output.imports),
};
// Only parse routes otherwise dynamic imports can fall into here and fail the build
} else if (output.entryPoint.startsWith("browser-route-module:")) {
let entryPointFile = output.entryPoint.replace(
/(^browser-route-module:|\?browser$)/g,
""
);
let route = routesByFile.get(entryPointFile);
invariant(route, `Cannot get route for entry point ${output.entryPoint}`);
let sourceExports = await getRouteModuleExportsCached(config, route.id);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import * as path from "path";
import type esbuild from "esbuild";

import type { RemixConfig } from "../../config";
Expand Down Expand Up @@ -31,7 +30,7 @@ export function browserRouteModulesPlugin(
let routesByFile: Map<string, Route> = Object.keys(config.routes).reduce(
(map, key) => {
let route = config.routes[key];
map.set(path.resolve(config.appDirectory, route.file), route);
map.set(route.file, route);
return map;
},
new Map()
Expand Down Expand Up @@ -71,12 +70,12 @@ export function browserRouteModulesPlugin(
let contents = "module.exports = {};";
if (theExports.length !== 0) {
let spec = `{ ${theExports.join(", ")} }`;
contents = `export ${spec} from ${JSON.stringify(file)};`;
contents = `export ${spec} from ${JSON.stringify(`./${file}`)};`;
}

return {
contents,
resolveDir: path.dirname(file),
resolveDir: config.appDirectory,
loader: "js",
};
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import * as path from "path";
import type { Plugin } from "esbuild";

import type { RemixConfig } from "../../config";
Expand Down Expand Up @@ -31,14 +30,12 @@ export function serverEntryModulePlugin(config: RemixConfig): Plugin {
resolveDir: config.appDirectory,
loader: "js",
contents: `
import * as entryServer from ${JSON.stringify(
path.resolve(config.appDirectory, config.entryServerFile)
)};
import * as entryServer from ${JSON.stringify(`./${config.entryServerFile}`)};
${Object.keys(config.routes)
.map((key, index) => {
let route = config.routes[key];
return `import * as route${index} from ${JSON.stringify(
path.resolve(config.appDirectory, route.file)
`./${route.file}`
)};`;
})
.join("\n")}
Expand Down

0 comments on commit 7a2188d

Please sign in to comment.