From f756ede3c85836889e0f1fb77ba8961fbc9dd2bf Mon Sep 17 00:00:00 2001 From: Jon Jensen Date: Fri, 18 Feb 2022 12:09:26 -0700 Subject: [PATCH] fix(remix-dev): relativize route modules to make builds deterministic Fixes #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 --- .../deterministic-build-output-test.ts | 28 ++++++++++ integration/helpers/create-fixture.ts | 8 +-- packages/remix-dev/compiler.ts | 5 +- packages/remix-dev/compiler/assets.ts | 18 +++---- .../plugins/browserRouteModulesPlugin.ts | 7 ++- .../plugins/serverEntryModulePlugin.ts | 7 +-- .../compiler/plugins/yarnPnpPlugin.ts | 52 +++++++++++++++++++ 7 files changed, 101 insertions(+), 24 deletions(-) create mode 100644 integration/deterministic-build-output-test.ts create mode 100644 packages/remix-dev/compiler/plugins/yarnPnpPlugin.ts diff --git a/integration/deterministic-build-output-test.ts b/integration/deterministic-build-output-test.ts new file mode 100644 index 00000000000..f713717491d --- /dev/null +++ b/integration/deterministic-build-output-test.ts @@ -0,0 +1,28 @@ +import { test, expect } from "@playwright/test"; +import globby from "globby"; +import fs from "fs"; +import path from "path"; + +import { createFixtureProject } 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])) + ); + }); +}); diff --git a/integration/helpers/create-fixture.ts b/integration/helpers/create-fixture.ts index 94af6ab020c..911fb62ff9e 100644 --- a/integration/helpers/create-fixture.ts +++ b/integration/helpers/create-fixture.ts @@ -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"; } @@ -143,7 +143,9 @@ export async function createAppFixture(fixture: Fixture) { } //////////////////////////////////////////////////////////////////////////////// -export async function createFixtureProject(init: FixtureInit): Promise { +export async function createFixtureProject( + init: FixtureInit = {} +): Promise { let template = init.template ?? "node-template"; let integrationTemplateDir = path.join(__dirname, template); let projectName = `remix-${template}-${Math.random().toString(32).slice(2)}`; @@ -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])); diff --git a/packages/remix-dev/compiler.ts b/packages/remix-dev/compiler.ts index 6ddc2fa68e8..72968c36a76 100644 --- a/packages/remix-dev/compiler.ts +++ b/packages/remix-dev/compiler.ts @@ -5,7 +5,6 @@ import * as fse from "fs-extra"; import debounce from "lodash.debounce"; import chokidar from "chokidar"; import { NodeModulesPolyfillPlugin } from "@esbuild-plugins/node-modules-polyfill"; -import { pnpPlugin as yarnPnpPlugin } from "@yarnpkg/esbuild-plugin-pnp"; import { BuildMode, BuildTarget } from "./build"; import type { RemixConfig } from "./config"; @@ -25,6 +24,7 @@ import { serverEntryModulePlugin } from "./compiler/plugins/serverEntryModulePlu import { serverRouteModulesPlugin } from "./compiler/plugins/serverRouteModulesPlugin"; import { writeFileSafe } from "./compiler/utils/fs"; import { urlImportsPlugin } from "./compiler/plugins/urlImportsPlugin"; +import { yarnPnpPlugin } from "./compiler/plugins/yarnPnpPlugin"; // When we build Remix, this shim file is copied directly into the output // directory in the same place relative to this file. It is eventually injected @@ -344,8 +344,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 = [ diff --git a/packages/remix-dev/compiler/assets.ts b/packages/remix-dev/compiler/assets.ts index 44185bcbc61..678a2f93f7d 100644 --- a/packages/remix-dev/compiler/assets.ts +++ b/packages/remix-dev/compiler/assets.ts @@ -59,7 +59,7 @@ export async function createAssetsManifest( let routesByFile: Map = 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() @@ -72,19 +72,19 @@ export async function createAssetsManifest( let output = metafile.outputs[key]; if (!output.entryPoint) continue; - let entryPointFile = path.resolve( - output.entryPoint.replace( - /(^browser-route-module:|^pnp:|\?browser$)/g, - "" - ) - ); - if (entryPointFile === entryClientFile) { + // When using yarn-pnp, esbuild-plugin-pnp resolves files under the pnp namespace, even entry.client.tsx + let entryPointFile = output.entryPoint.replace(/^pnp:/, ""); + if (path.resolve(entryPointFile) === 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:")) { + } else if (entryPointFile.startsWith("browser-route-module:")) { + entryPointFile = entryPointFile.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); diff --git a/packages/remix-dev/compiler/plugins/browserRouteModulesPlugin.ts b/packages/remix-dev/compiler/plugins/browserRouteModulesPlugin.ts index fb2ed3ff695..6e66da4aef1 100644 --- a/packages/remix-dev/compiler/plugins/browserRouteModulesPlugin.ts +++ b/packages/remix-dev/compiler/plugins/browserRouteModulesPlugin.ts @@ -1,4 +1,3 @@ -import * as path from "path"; import type esbuild from "esbuild"; import type { RemixConfig } from "../../config"; @@ -31,7 +30,7 @@ export function browserRouteModulesPlugin( let routesByFile: Map = 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() @@ -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", }; } diff --git a/packages/remix-dev/compiler/plugins/serverEntryModulePlugin.ts b/packages/remix-dev/compiler/plugins/serverEntryModulePlugin.ts index 79e20f4f419..662cfe769df 100644 --- a/packages/remix-dev/compiler/plugins/serverEntryModulePlugin.ts +++ b/packages/remix-dev/compiler/plugins/serverEntryModulePlugin.ts @@ -1,4 +1,3 @@ -import * as path from "path"; import type { Plugin } from "esbuild"; import type { RemixConfig } from "../../config"; @@ -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")} diff --git a/packages/remix-dev/compiler/plugins/yarnPnpPlugin.ts b/packages/remix-dev/compiler/plugins/yarnPnpPlugin.ts new file mode 100644 index 00000000000..37b46d1d2ed --- /dev/null +++ b/packages/remix-dev/compiler/plugins/yarnPnpPlugin.ts @@ -0,0 +1,52 @@ +import type { + Plugin, + OnResolveOptions, + OnResolveArgs, + OnResolveResult, + PluginBuild, +} from "esbuild"; +import { pnpPlugin } from "@yarnpkg/esbuild-plugin-pnp"; + +// esbuild-plugin-pnp doesn't correctly handle imports that happen within virtual modules +// with relative paths; it assumes that `importer` refers to a real path, and incorrectly +// resolves modules based on it. It should respect the resolveDir if present. +// See here: https://esbuild.github.io/plugins/#on-resolve-arguments +// +// Ideally we could just use an esbuild filter regex so that it only resolves module specifiers +// and their dependencies and *not* project relative imports, but we can't because there's +// no way to distinguish those from dependencies' relative imports which we *do* want yarn-pnp +// to handle. So anything not resolved by earlier plugins has to be resolved by yarn-pnp. +// +// This will be fixed by https://github.com/yarnpkg/berry/pull/4569 +// but in the meantime we need this hack 🙃 +export const yarnPnpPlugin = (): Plugin => { + return { + name: "yarn-pnp", + setup(build) { + return pnpPlugin().setup( + new Proxy(build, { + get(target, property) { + if (property === "onResolve") { + return ( + options: OnResolveOptions, + pluginOnResolveCallback: ( + args: OnResolveArgs + ) => OnResolveResult + ) => { + build.onResolve(options, (args: any) => { + // sneakily fix the importer before pnp onResolve callback runs + if (args.resolveDir) { + args.importer = args.resolveDir + "/"; + } + return pluginOnResolveCallback(args); + }); + }; + } else { + return target[property as keyof PluginBuild]; + } + }, + }) + ); + }, + }; +};