From ca616240a9446944adb3e82e47357182bfee2359 Mon Sep 17 00:00:00 2001 From: Logan McAnsh Date: Tue, 5 Jul 2022 16:18:13 -0400 Subject: [PATCH 1/5] fix(serverBareModulesPlugin): update require.resolve to use full import, update warning Signed-off-by: Logan McAnsh --- .../remix-dev/compiler/plugins/serverBareModulesPlugin.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts b/packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts index 59ca2662dc9..b68254f04ba 100644 --- a/packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts +++ b/packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts @@ -71,12 +71,12 @@ export function serverBareModulesPlugin( !/\bnode_modules\b/.test(importer) ) { try { - require.resolve(packageName); + require.resolve(path); } catch (error) { onWarning( `The path "${path}" is imported in ` + `${relative(process.cwd(), importer)} but ` + - `${packageName} is not listed in your package.json dependencies. ` + + `${packageName} was not found in your node_modules. ` + `Did you forget to install it?`, packageName ); From dbe416391f6d26f1347f738d31330989c4d70bee Mon Sep 17 00:00:00 2001 From: Logan McAnsh Date: Tue, 5 Jul 2022 22:27:52 -0400 Subject: [PATCH 2/5] test: initial pass at writing a test for missing modules this test also covers installed modules as it uses @remix-run/node and @remix-run/react Signed-off-by: Logan McAnsh --- integration/compiler-test.ts | 68 ++++++++++++++++--- integration/helpers/create-fixture.ts | 14 ++-- .../plugins/serverBareModulesPlugin.ts | 2 +- 3 files changed, 67 insertions(+), 17 deletions(-) diff --git a/integration/compiler-test.ts b/integration/compiler-test.ts index 2242afa0f32..7e2504a698f 100644 --- a/integration/compiler-test.ts +++ b/integration/compiler-test.ts @@ -1,12 +1,14 @@ import path from "path"; import fs from "fs/promises"; import { test, expect } from "@playwright/test"; +import { PassThrough } from "stream"; import { createFixture, createAppFixture, js, json, + createFixtureProject, } from "./helpers/create-fixture"; import type { Fixture, AppFixture } from "./helpers/create-fixture"; import { PlaywrightFixture } from "./helpers/playwright-fixture"; @@ -134,6 +136,14 @@ test.describe("compiler", () => { module: "./esm/index.js", sideEffects: false, }), + "node_modules/@org/package/sub-package/index.js": js` + module.exports.submodule = require("./submodule.js"); + `, + "node_modules/@org/package/sub-package/submodule.js": js` + module.exports = function submodule() { + return "package-with-submodule"; + } + `, "node_modules/@org/package/sub-package/esm/package.json": json({ type: "module", sideEffects: false, @@ -146,14 +156,6 @@ test.describe("compiler", () => { return "package-with-submodule"; } `, - "node_modules/@org/package/sub-package/index.js": js` - module.exports.submodule = require("./submodule.js"); - `, - "node_modules/@org/package/sub-package/submodule.js": js` - module.exports = function submodule() { - return "package-with-submodule"; - } - `, }, }); @@ -320,4 +322,54 @@ test.describe("compiler", () => { expect(magicRemix).toContain(name); } }); + + test.describe("serverBareModulesPlugin", () => { + test("warns when a module isn't installed", async () => { + let buildOutput: string; + let buildStdio = new PassThrough(); + + await expect(() => + createFixtureProject({ + buildStdio, + files: { + "app/routes/index.jsx": js` + import { json } from "@remix-run/node"; + import { useLoaderData } from "@remix-run/react"; + import notInstalledMain from "some-not-installed-module"; + import { notInstalledSub } from "some-not-installed-module/sub"; + + export function loader() { + return json({ main: notInstalledMain(), sub: notInstalledSub() }); + } + + export default function Index() { + let data = useLoaderData(); + return null; + } + `, + }, + }) + ).rejects.toThrowError("Build failed, check the output above"); + + let chunks: Buffer[] = []; + buildOutput = await new Promise((resolve, reject) => { + buildStdio.on("error", (error) => { + reject(error); + }); + buildStdio.on("data", (chunk) => { + chunks.push(Buffer.from(chunk)); + }); + buildStdio.on("end", () => { + resolve(Buffer.concat(chunks).toString("utf8")); + }); + }); + + expect(buildOutput).toContain( + `The path "some-not-installed-module" is imported in app/routes/index.jsx but "some-not-installed-module" was not found in your node_modules. Did you forget to install it?` + ); + expect(buildOutput).toContain( + `The path "some-not-installed-module/sub" is imported in app/routes/index.jsx but "some-not-installed-module/sub" was not found in your node_modules. Did you forget to install it?` + ); + }); + }); }); diff --git a/integration/helpers/create-fixture.ts b/integration/helpers/create-fixture.ts index a3ca94c5ead..db61a7eb051 100644 --- a/integration/helpers/create-fixture.ts +++ b/integration/helpers/create-fixture.ts @@ -147,7 +147,6 @@ export async function createFixtureProject(init: FixtureInit): Promise { { overwrite: true } ); if (init.setup) { - // eslint-disable-next-line @typescript-eslint/no-unused-vars let setupSpawn = spawnSync( "node", ["node_modules/@remix-run/dev/dist/cli.js", "setup", init.setup], @@ -178,9 +177,7 @@ function build(projectDir: string, buildStdio?: Writable, sourcemap?: boolean) { if (sourcemap) { buildArgs.push("--sourcemap"); } - let buildSpawn = spawnSync("node", buildArgs, { - cwd: projectDir, - }); + let buildSpawn = spawnSync("node", buildArgs, { cwd: projectDir }); // These logs are helpful for debugging. Remove comments if needed. // console.log("spawning @remix-run/dev/cli.js `build`:\n"); @@ -188,16 +185,17 @@ function build(projectDir: string, buildStdio?: Writable, sourcemap?: boolean) { // console.log(" " + buildSpawn.stdout.toString("utf-8")); // console.log(" STDERR:"); // console.log(" " + buildSpawn.stderr.toString("utf-8")); - if (buildSpawn.error || buildSpawn.status) { - console.error(buildSpawn.stderr.toString("utf-8")); - throw buildSpawn.error || new Error(`Build failed, check the output above`); - } if (buildStdio) { buildStdio.write(buildSpawn.stdout.toString("utf-8")); buildStdio.write(buildSpawn.stderr.toString("utf-8")); buildStdio.end(); } + + if (buildSpawn.error || buildSpawn.status) { + console.error(buildSpawn.stderr.toString("utf-8")); + throw buildSpawn.error || new Error(`Build failed, check the output above`); + } } async function writeTestFiles(init: FixtureInit, dir: string) { diff --git a/packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts b/packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts index b68254f04ba..74e0f57eae9 100644 --- a/packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts +++ b/packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts @@ -76,7 +76,7 @@ export function serverBareModulesPlugin( onWarning( `The path "${path}" is imported in ` + `${relative(process.cwd(), importer)} but ` + - `${packageName} was not found in your node_modules. ` + + `"${path}" was not found in your node_modules. ` + `Did you forget to install it?`, packageName ); From 3b601b4d1a5580a37f572f6b957666757aa55b4d Mon Sep 17 00:00:00 2001 From: Logan McAnsh Date: Mon, 11 Jul 2022 12:27:27 -0400 Subject: [PATCH 3/5] fix: use path as warning key Signed-off-by: Logan McAnsh --- packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts b/packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts index 74e0f57eae9..0ace01c072f 100644 --- a/packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts +++ b/packages/remix-dev/compiler/plugins/serverBareModulesPlugin.ts @@ -78,7 +78,7 @@ export function serverBareModulesPlugin( `${relative(process.cwd(), importer)} but ` + `"${path}" was not found in your node_modules. ` + `Did you forget to install it?`, - packageName + path ); } } From 6b18244a6c1668507c8a4e4ba0005df84239166c Mon Sep 17 00:00:00 2001 From: Logan McAnsh Date: Mon, 11 Jul 2022 12:29:24 -0400 Subject: [PATCH 4/5] Create yellow-flowers-trade.md --- .changeset/yellow-flowers-trade.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/yellow-flowers-trade.md diff --git a/.changeset/yellow-flowers-trade.md b/.changeset/yellow-flowers-trade.md new file mode 100644 index 00000000000..d77fd00be3c --- /dev/null +++ b/.changeset/yellow-flowers-trade.md @@ -0,0 +1,5 @@ +--- +"@remix-run/dev": patch +--- + +update serverBareModulesPlugin warning to use full import path - say dependency isn't available in node_modules instead of package.json From 09008c602da17ea6cd70962a078ca367057e20e7 Mon Sep 17 00:00:00 2001 From: Logan McAnsh Date: Mon, 11 Jul 2022 13:17:28 -0400 Subject: [PATCH 5/5] test: path.join for importer Signed-off-by: Logan McAnsh --- integration/compiler-test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/integration/compiler-test.ts b/integration/compiler-test.ts index 7e2504a698f..b33e5ddc6bd 100644 --- a/integration/compiler-test.ts +++ b/integration/compiler-test.ts @@ -364,11 +364,13 @@ test.describe("compiler", () => { }); }); + let importer = path.join("app", "routes", "index.jsx"); + expect(buildOutput).toContain( - `The path "some-not-installed-module" is imported in app/routes/index.jsx but "some-not-installed-module" was not found in your node_modules. Did you forget to install it?` + `The path "some-not-installed-module" is imported in ${importer} but "some-not-installed-module" was not found in your node_modules. Did you forget to install it?` ); expect(buildOutput).toContain( - `The path "some-not-installed-module/sub" is imported in app/routes/index.jsx but "some-not-installed-module/sub" was not found in your node_modules. Did you forget to install it?` + `The path "some-not-installed-module/sub" is imported in ${importer} but "some-not-installed-module/sub" was not found in your node_modules. Did you forget to install it?` ); }); });