fix(remix-dev): relativize route modules to make builds deterministic

[jenseng]

Closes #2024

• Tests

This fixes an issue where the build manifest/hashes/etc will differ based on the parent directory structure. If virtual modules import from absolute paths, their content is technically different when those paths change, so 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). Sometimes you can't directly control the absolute path where you code is checked out and run (e.g. Jenkins, docker, etc.).

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

[kentcdodds]

I'm surprised that test case fails without your changes (I'm assuming you ran the test first to ensure it does fail without your changes). This is beyond where I'm comfortable merging personally, but I did leave a bit of feedback if you'd like :) Thanks!

[kentcdodds]

Thanks for doing that!

I'll leave the rest of the review to @mjackson

[jenseng]

Seems like an unrelated CLI test failed, is there a way to retrigger? Or does it not matter?

[jenseng]

@mjackson let me know if you have any questions/suggestions 😄

[ryanflorence]

Let's fix the conflicts and then merge this bugger.

[ryanflorence]

Jon has local changes but Yarn PnP doesn't play nicely with relative paths, so going to wait for a fix there before we do this.

[jenseng]

Alright, I opened a PR to fix the esbuild-plugin-pnp bug, but if we're comfortable shipping this hack we don't technically need to wait for it 😅

[jenseng]

The esbuild-plugin-pnp bugfix has been merged, so once they release a new version I'll unwind my hack and bump the dependency 🥳

[jenseng]

This PR should be good to go, but it looks like something is generally broken around the caching done by bahmutov\npm-install step in GH actions... if a build can't use the cache (i.e. yarn.lock has changed), the windows builds will fails. Here's a simple repro I set up that fails the same way: #3584

[jenseng]

Alright, the PR now uses @yarnpkg/esbuild-plugin-pnp/3.0.0-rc.10 which lets us remove the plugin hack/wrapper... in addition to the tests still all passing, examples/yarn-pnp also still builds correctly with this change.

@yarnpkg/esbuild-plugin-pnp/3.x does require node >=14.15.0 (the first 14.x LTS version), but that's not new for remix since @npmcli/package-json (another dependency) does too.

That said, this all may be moot as I just noticed this PR which swaps out this plugin entirely 😆😭 ... in any event, here's the diff from 2.0.2 (what people would have been installing prior to this PR). Although the version we're using is an RC, IMO it's safe to use since the only change it brings is my bugfix:

diff --git a/packages/esbuild-plugin-pnp/package.json b/packages/esbuild-plugin-pnp/package.json
index dbb346c74..8f22e8fb6 100644
--- a/packages/esbuild-plugin-pnp/package.json
+++ b/packages/esbuild-plugin-pnp/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@yarnpkg/esbuild-plugin-pnp",
-  "version": "2.0.2",
+  "version": "3.0.0-rc.10",
   "license": "BSD-2-Clause",
   "main": "./sources/index.ts",
   "dependencies": {
@@ -31,6 +31,7 @@
     "directory": "packages/esbuild-plugin-pnp"
   },
   "engines": {
-    "node": ">=12 <14 || 14.2 - 14.9 || >14.10.0"
-  }
+    "node": ">=14.15.0"
+  },
+  "stableVersion": "2.0.1"
 }
diff --git a/packages/esbuild-plugin-pnp/sources/index.ts b/packages/esbuild-plugin-pnp/sources/index.ts
index 379b64959..64fff1588 100644
--- a/packages/esbuild-plugin-pnp/sources/index.ts
+++ b/packages/esbuild-plugin-pnp/sources/index.ts
@@ -139,9 +139,11 @@ export function pnpPlugin({
           conditions = conditionsRequire;

         // The entry point resolution uses an empty string
-        const effectiveImporter = args.importer
-          ? args.importer
-          : `${baseDir}/`;
+        const effectiveImporter = args.resolveDir
+          ? `${args.resolveDir}/`
+          : args.importer
+            ? args.importer
+            : `${baseDir}/`;

         const pnpApi = findPnpApi(effectiveImporter) as PnpApi | null;
         if (!pnpApi)

[MichaelDeBoey]

As mentioned in #3594 (comment), I personally would be in favor of solving the problems from #3594 in @yarnpkg/esbuild-plugin-pnp (like we did for this one) & update the dependency instead of removing the plugin & having extra maintenance on our side tbh.

@jenseng Maybe you could point @lensbart into the right direction for a fix in @yarnpkg/esbuild-plugin-pnp?
I think you creating a fix yourself could also work though.

[MichaelDeBoey]

@jenseng Due to merging #3633, we have some conflicts here. Could you please resolve them?

[changeset-bot]

🦋 Changeset detected

Latest commit: 1c6f49e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages

Name	Type
@remix-run/dev	Patch
create-remix	Patch
remix	Patch
@remix-run/eslint-config	Patch
@remix-run/react	Patch
@remix-run/serve	Patch
@remix-run/server-runtime	Patch
@remix-run/cloudflare	Patch
@remix-run/node	Patch
@remix-run/deno	Patch
@remix-run/cloudflare-pages	Patch
@remix-run/cloudflare-workers	Patch
@remix-run/express	Patch
@remix-run/netlify	Patch
@remix-run/vercel	Patch
@remix-run/architect	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR