Skip to content

Commit

Permalink
Merge branch 'dev' into markdalgleish/polyfill-package-imports-in-deps
Browse files Browse the repository at this point in the history
  • Loading branch information
markdalgleish authored Aug 22, 2023
2 parents 424a444 + cdd6927 commit 05364f8
Show file tree
Hide file tree
Showing 10 changed files with 18 additions and 126 deletions.
73 changes: 0 additions & 73 deletions integration/compiler-test.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
import path from "node:path";
import fse from "fs-extra";
import { test, expect } from "@playwright/test";
import { PassThrough } from "node:stream";

import {
createFixture,
createAppFixture,
js,
json,
createFixtureProject,
css,
} from "./helpers/create-fixture.js";
import type { Fixture, AppFixture } from "./helpers/create-fixture.js";
Expand Down Expand Up @@ -310,75 +308,4 @@ test.describe("compiler", () => {
expect(cssFile).toBeTruthy();
expect(fontFile).toBeTruthy();
});

test.describe("serverBareModulesPlugin", () => {
let ogConsole: typeof global.console;
test.beforeEach(() => {
ogConsole = global.console;
// @ts-ignore
global.console = {
log() {},
warn() {},
error() {},
};
});
test.afterEach(() => {
global.console = ogConsole;
});
test("warns when a module isn't installed", async () => {
let buildOutput: string;
let buildStdio = new PassThrough();

await expect(() =>
createFixtureProject({
buildStdio,
files: {
"app/routes/_index.tsx": 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.tsx");

expect(buildOutput).toContain(
`could not resolve "some-not-installed-module"`
);
expect(buildOutput).toContain(
`You imported "some-not-installed-module" in ${importer},`
);
expect(buildOutput).toContain(
`could not resolve "some-not-installed-module/sub"`
);
expect(buildOutput).toContain(
`You imported "some-not-installed-module/sub" in ${importer},`
);
});
});
});
10 changes: 9 additions & 1 deletion packages/create-remix/copy-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,15 @@ async function downloadAndExtractTarball(
header.name = header.name.replace(`${originalDirName}/`, "");

if (filePath) {
if (header.name.startsWith(filePath)) {
// Include trailing slash on startsWith when filePath doesn't include
// it so something like `templates/remix` doesn't inadvertently
// include `templates/remix-javascript/*` files
if (
(filePath.endsWith(path.posix.sep) &&
header.name.startsWith(filePath)) ||
(!filePath.endsWith(path.posix.sep) &&
header.name.startsWith(filePath + path.posix.sep))
) {
filePathHasFiles = true;
header.name = header.name.replace(filePath, "");
} else {
Expand Down
8 changes: 4 additions & 4 deletions packages/remix-deno/sessions/fileStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export function createFileSessionStorage<Data = SessionData, FlashData = Data>({
}: FileSessionStorageOptions): SessionStorage<Data, FlashData> {
return createSessionStorage({
cookie,
async createData(data, expires) {
createData: async (data, expires) => {
const content = JSON.stringify({ data, expires });

while (true) {
Expand Down Expand Up @@ -65,7 +65,7 @@ export function createFileSessionStorage<Data = SessionData, FlashData = Data>({
}
}
},
async readData(id) {
readData: async (id) => {
try {
const file = getFile(dir, id);
const content = JSON.parse(await Deno.readTextFile(file));
Expand All @@ -87,13 +87,13 @@ export function createFileSessionStorage<Data = SessionData, FlashData = Data>({
return null;
}
},
async updateData(id, data, expires) {
updateData: async (id, data, expires) => {
const content = JSON.stringify({ data, expires });
const file = getFile(dir, id);
await Deno.mkdir(path.dirname(file), { recursive: true }).catch(() => {});
await Deno.writeTextFile(file, content);
},
async deleteData(id) {
deleteData: async (id) => {
try {
await Deno.remove(getFile(dir, id));
} catch (error) {
Expand Down
45 changes: 1 addition & 44 deletions packages/remix-dev/compiler/server/plugins/bareImports.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { isAbsolute, relative } from "node:path";
import { builtinModules } from "node:module";
import { isAbsolute } from "node:path";
import type { Plugin } from "esbuild";

import {
Expand All @@ -8,7 +7,6 @@ import {
} from "../virtualModules";
import { isCssSideEffectImportPath } from "../../plugins/cssSideEffectImports";
import { createMatchPath } from "../../utils/tsconfig";
import { detectPackageManager } from "../../../cli/detectPackageManager";
import type { Context } from "../../context";
import { getLoaderForFile } from "../../utils/loaders";

Expand Down Expand Up @@ -85,36 +83,6 @@ export function serverBareModulesPlugin(ctx: Context): Plugin {
return undefined;
}

let packageName = getNpmPackageName(path);
let pkgManager = detectPackageManager() ?? "npm";

// Warn if we can't find an import for a package.
if (
!isNodeBuiltIn(packageName) &&
!/\bnode_modules\b/.test(importer) &&
// Silence spurious warnings when using Yarn PnP. Yarn PnP doesn’t use
// a `node_modules` folder to keep its dependencies, so the above check
// will always fail.
(pkgManager === "npm" ||
(pkgManager === "yarn" && process.versions.pnp == null))
) {
try {
require.resolve(path, { paths: [importer] });
} catch (error: unknown) {
ctx.logger.warn(`could not resolve "${path}"`, {
details: [
`You imported "${path}" in ${relative(
process.cwd(),
importer
)},`,
"but that package is not in your `node_modules`.",
"Did you forget to install it?",
],
key: path,
});
}
}

if (ctx.config.serverDependenciesToBundle === "all") {
return undefined;
}
Expand All @@ -138,17 +106,6 @@ export function serverBareModulesPlugin(ctx: Context): Plugin {
};
}

function isNodeBuiltIn(packageName: string) {
return builtinModules.includes(packageName);
}

function getNpmPackageName(id: string): string {
let split = id.split("/");
let packageName = split[0];
if (packageName.startsWith("@")) packageName += `/${split[1]}`;
return packageName;
}

function isBareModuleId(id: string): boolean {
return !id.startsWith("node:") && !id.startsWith(".") && !isAbsolute(id);
}
File renamed without changes.
1 change: 1 addition & 0 deletions templates/remix-javascript/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"private": true,
"sideEffects": false,
"type": "module",
"scripts": {
"build": "remix build",
"dev": "remix dev",
Expand Down
3 changes: 1 addition & 2 deletions templates/remix-javascript/remix.config.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
/** @type {import('@remix-run/dev').AppConfig} */
module.exports = {
export default {
ignoredRouteFiles: ["**/.*"],
// appDirectory: "app",
// assetsBuildDirectory: "public/build",
// serverBuildPath: "build/index.js",
// publicPath: "/build/",
serverModuleFormat: "cjs",
};
File renamed without changes.
1 change: 1 addition & 0 deletions templates/remix/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"private": true,
"sideEffects": false,
"type": "module",
"scripts": {
"build": "remix build",
"dev": "remix dev",
Expand Down
3 changes: 1 addition & 2 deletions templates/remix/remix.config.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
/** @type {import('@remix-run/dev').AppConfig} */
module.exports = {
export default {
ignoredRouteFiles: ["**/.*"],
// appDirectory: "app",
// assetsBuildDirectory: "public/build",
// serverBuildPath: "build/index.js",
// publicPath: "/build/",
serverModuleFormat: "cjs",
};

0 comments on commit 05364f8

Please sign in to comment.