Skip to content

Commit

Permalink
fix(serverBareModulesPlugin): update require.resolve to use full impo…
Browse files Browse the repository at this point in the history
…rt, update warning (#3656)
  • Loading branch information
mcansh authored Jul 11, 2022
1 parent 55bb00e commit de9fc05
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 19 deletions.
5 changes: 5 additions & 0 deletions .changeset/yellow-flowers-trade.md
Original file line number Diff line number Diff line change
@@ -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
70 changes: 62 additions & 8 deletions integration/compiler-test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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,
Expand All @@ -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";
}
`,
},
});

Expand Down Expand Up @@ -320,4 +322,56 @@ 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<string>((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"));
});
});

let importer = path.join("app", "routes", "index.jsx");

expect(buildOutput).toContain(
`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 ${importer} but "some-not-installed-module/sub" was not found in your node_modules. Did you forget to install it?`
);
});
});
});
14 changes: 6 additions & 8 deletions integration/helpers/create-fixture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ export async function createFixtureProject(init: FixtureInit): Promise<string> {
{ 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],
Expand Down Expand Up @@ -178,26 +177,25 @@ 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");
// console.log(" STDOUT:");
// 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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,14 @@ 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. ` +
`"${path}" was not found in your node_modules. ` +
`Did you forget to install it?`,
packageName
path
);
}
}
Expand Down

0 comments on commit de9fc05

Please sign in to comment.