Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(dev): prettier logging for remix build and remix dev #6596

Merged
merged 12 commits into from
Jun 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/orange-beds-explode.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/dev": minor
---

improved logging for `remix build` and `remix dev`
10 changes: 8 additions & 2 deletions integration/compiler-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -428,10 +428,16 @@ test.describe("compiler", () => {
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?`
`could not resolve "some-not-installed-module"`
);
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?`
`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},`
);
});
});
Expand Down
28 changes: 7 additions & 21 deletions integration/esm-only-warning-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,25 +179,11 @@ test.beforeAll(async () => {
});

test("logs warnings for ESM only packages", async () => {
expect(buildOutput).toContain(
"esm-only-no-exports is possibly an ESM only package"
);
expect(buildOutput).toContain(
"esm-only-exports is possibly an ESM only package"
);
expect(buildOutput).not.toContain(
"esm-only-exports-b is possibly an ESM only package"
);
expect(buildOutput).not.toContain(
"esm-only-exports-c is possibly an ESM only package"
);
expect(buildOutput).not.toContain(
"cjs-dynamic-import is possibly an ESM only package"
);
expect(buildOutput).toContain(
"esm-only-sub-exports is possibly an ESM only package"
);
expect(buildOutput).not.toContain(
"esm-cjs-exports is possibly an ESM only package"
);
expect(buildOutput).toContain("esm-only package: esm-only-no-exports");
expect(buildOutput).toContain("esm-only package: esm-only-exports");
expect(buildOutput).not.toContain("esm-only package: esm-only-exports-b");
expect(buildOutput).not.toContain("esm-only package: esm-only-exports-c");
expect(buildOutput).not.toContain("esm-only package: cjs-dynamic-import");
expect(buildOutput).toContain("esm-only package: esm-only-sub-exports");
expect(buildOutput).not.toContain("esm-only package: esm-cjs-exports");
});
7 changes: 4 additions & 3 deletions integration/flat-routes-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { PlaywrightFixture } from "./helpers/playwright-fixture";
import type { Fixture, AppFixture } from "./helpers/create-fixture";
import { createFixtureProject } from "./helpers/create-fixture";
import { createAppFixture, createFixture, js } from "./helpers/create-fixture";
import { flatRoutesWarning } from "../packages/remix-dev/config";

let fixture: Fixture;
let appFixture: AppFixture;
Expand Down Expand Up @@ -159,7 +158,7 @@ test.describe("flat routes", () => {
});
}

test("allows ignoredRouteFiles to be configured", async ({ page }) => {
test("allows ignoredRouteFiles to be configured", async () => {
let routeIds = Object.keys(fixture.build.routes);

expect(routeIds).not.toContain(IGNORED_ROUTE);
Expand Down Expand Up @@ -210,7 +209,9 @@ test.describe("warns when v1 routesConvention is used", () => {

test("v2_routeConvention is not enabled", () => {
console.log(buildOutput);
expect(buildOutput).toContain(flatRoutesWarning);
expect(buildOutput).toContain(
"The route file convention is changing in v2"
);
});
});

Expand Down
24 changes: 2 additions & 22 deletions packages/remix-dev/__tests__/create-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,6 @@ import stripAnsi from "strip-ansi";

import { run } from "../cli/run";
import { server } from "./msw";
import {
errorBoundaryWarning,
flatRoutesWarning,
formMethodWarning,
headersWarning,
metaWarning,
serverModuleFormatWarning,
} from "../config";

beforeAll(() => server.listen({ onUnhandledRequest: "error" }));
afterAll(() => server.close());
Expand Down Expand Up @@ -354,20 +346,8 @@ describe("the create command", () => {
"--no-install",
"--no-typescript",
]);
expect(output.trim()).toBe(
errorBoundaryWarning +
"\n" +
formMethodWarning +
"\n" +
metaWarning +
"\n" +
headersWarning +
"\n" +
serverModuleFormatWarning +
"\n" +
flatRoutesWarning +
"\n\n" +
getOptOutOfInstallMessage() +
expect(output.trim()).toContain(
getOptOutOfInstallMessage() +
"\n\n" +
getSuccessMessage(path.join("<TEMP_DIR>", "template-to-js"))
);
Expand Down
8 changes: 1 addition & 7 deletions packages/remix-dev/__tests__/readConfig-test.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,17 @@
import path from "path";

import type { RemixConfig } from "../config";
import { serverBuildTargetWarning, readConfig } from "../config";
import { readConfig } from "../config";

const remixRoot = path.resolve(__dirname, "./fixtures/stack");

describe("readConfig", () => {
let config: RemixConfig;
let warnStub;
beforeEach(async () => {
let consoleWarn = console.warn;
warnStub = jest.fn();
console.warn = warnStub;
config = await readConfig(remixRoot);
console.warn = consoleWarn;
});

it("generates a config", async () => {
expect(warnStub).toHaveBeenCalledWith(serverBuildTargetWarning);
expect(config).toMatchInlineSnapshot(
{
rootDirectory: expect.any(String),
Expand Down
49 changes: 24 additions & 25 deletions packages/remix-dev/cli/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import prettyMs from "pretty-ms";
import * as esbuild from "esbuild";
import NPMCliPackageJson from "@npmcli/package-json";
import { coerce } from "semver";
import pc from "picocolors";

import * as colors from "../colors";
import * as compiler from "../compiler";
Expand All @@ -23,9 +24,9 @@ import runCodemod from "../codemod";
import { CodemodError } from "../codemod/utils/error";
import { TaskError } from "../codemod/utils/task";
import { transpile as convertFileToJS } from "./useJavascript";
import { warnOnce } from "../warnOnce";
import type { Options } from "../compiler/options";
import { createFileWatchCache } from "../compiler/fileWatchCache";
import { logger } from "../tux";

export async function create({
appTemplate,
Expand Down Expand Up @@ -152,29 +153,25 @@ export async function build(
): Promise<void> {
let mode = parseMode(modeArg) ?? "production";

console.log(`Building Remix app in ${mode} mode...`);
logger.info(`building...` + pc.gray(` (NODE_ENV=${mode})`));

if (modeArg === "production" && sourcemap) {
console.warn(
"\n⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️"
);
console.warn(
"You have enabled source maps in production. This will make your " +
"server-side code visible to the public and is highly discouraged! If " +
"you insist, please ensure you are using environment variables for " +
"secrets and not hard-coding them into your source!"
);
console.warn(
"⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️\n"
);
logger.warn("🚨 source maps enabled in production", {
details: [
"You are using `--sourcemap` to enable source maps in production,",
"making your server-side code publicly visible in the browser.",
"This is highly discouraged!",
"If you insist, ensure that you are using environment variables for secrets",
"and are not hard-coding them in your source.",
],
});
}

let start = Date.now();
let config = await readConfig(remixRoot);
let options: Options = {
mode,
sourcemap,
onWarning: warnOnce,
};
if (mode === "development" && config.future.unstable_dev) {
let origin = await resolveDevOrigin(config);
Expand All @@ -184,12 +181,14 @@ export async function build(
let fileWatchCache = createFileWatchCache();

fse.emptyDirSync(config.assetsBuildDirectory);
await compiler.build({ config, options, fileWatchCache }).catch((thrown) => {
compiler.logThrown(thrown);
process.exit(1);
});
await compiler
.build({ config, options, fileWatchCache, logger })
.catch((thrown) => {
compiler.logThrown(thrown);
process.exit(1);
});

console.log(`Built in ${prettyMs(Date.now() - start)}`);
logger.info("built" + pc.gray(` (${prettyMs(Date.now() - start)})`));
}

// TODO: replace watch in v2
Expand Down Expand Up @@ -224,12 +223,12 @@ export async function dev(
tlsCert?: string;
} = {}
) {
// clear screen
process.stdout.write("\x1Bc");
console.log(`\n 💿 remix dev\n`);

if (process.env.NODE_ENV && process.env.NODE_ENV !== "development") {
console.warn(
`Forcing NODE_ENV to be 'development'. Was: ${JSON.stringify(
process.env.NODE_ENV
)}`
);
logger.warn(`overriding NODE_ENV=${process.env.NODE_ENV} to development`);
}
process.env.NODE_ENV = "development";
if (flags.debug) inspector.open();
Expand Down
2 changes: 2 additions & 0 deletions packages/remix-dev/compiler/context.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import type { RemixConfig } from "../config";
import type { Logger } from "../tux";
import type { FileWatchCache } from "./fileWatchCache";
import type { Options } from "./options";

export type Context = {
config: RemixConfig;
options: Options;
fileWatchCache: FileWatchCache;
logger: Logger;
};
77 changes: 1 addition & 76 deletions packages/remix-dev/compiler/js/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,10 @@ import { mdxPlugin } from "../plugins/mdx";
import { externalPlugin } from "../plugins/external";
import { cssBundlePlugin } from "../plugins/cssBundlePlugin";
import { cssModulesPlugin } from "../plugins/cssModuleImports";
import {
cssSideEffectImportsPlugin,
isCssSideEffectImportPath,
} from "../plugins/cssSideEffectImports";
import { cssSideEffectImportsPlugin } from "../plugins/cssSideEffectImports";
import { vanillaExtractPlugin } from "../plugins/vanillaExtract";
import invariant from "../../invariant";
import { hmrPlugin } from "./plugins/hmr";
import { createMatchPath } from "../utils/tsconfig";
import { detectPackageManager } from "../../cli/detectPackageManager";
import type { LazyValue } from "../lazyValue";
import type { Context } from "../context";

Expand All @@ -38,21 +33,6 @@ type Compiler = {
dispose: () => Promise<void>;
};

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(".") && !path.isAbsolute(id);
}

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

const getExternals = (remixConfig: RemixConfig): string[] => {
// For the browser build, exclude node built-ins that don't have a
// browser-safe alternative installed in node_modules. Nothing should
Expand Down Expand Up @@ -105,18 +85,6 @@ const createEsbuildConfig = (
);
}

let matchPath = ctx.config.tsconfigPath
? createMatchPath(ctx.config.tsconfigPath)
: undefined;
function resolvePath(id: string) {
if (!matchPath) {
return id;
}
return (
matchPath(id, undefined, undefined, [".ts", ".tsx", ".js", ".jsx"]) || id
);
}

let plugins: esbuild.Plugin[] = [
browserRouteModulesPlugin(ctx, /\?browser$/),
deprecatedRemixPackagePlugin(ctx),
Expand All @@ -135,49 +103,6 @@ const createEsbuildConfig = (
emptyModulesPlugin(ctx, /\.server(\.[jt]sx?)?$/),
nodeModulesPolyfillPlugin(),
externalPlugin(/^node:.*/, { sideEffects: false }),
{
// TODO: should be removed when error handling for compiler is improved
name: "warn-on-unresolved-imports",
setup: (build) => {
build.onResolve({ filter: /.*/ }, (args) => {
if (!isBareModuleId(resolvePath(args.path))) {
return undefined;
}

if (args.path === "remix:hmr") {
return undefined;
}

let packageName = getNpmPackageName(args.path);
let pkgManager = detectPackageManager() ?? "npm";
if (
ctx.options.onWarning &&
!isNodeBuiltIn(packageName) &&
!/\bnode_modules\b/.test(args.importer) &&
!args.path.endsWith(".css") &&
!isCssSideEffectImportPath(args.path) &&
// 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(args.path);
} catch (error: unknown) {
ctx.options.onWarning(
`The path "${args.path}" is imported in ` +
`${path.relative(process.cwd(), args.importer)} but ` +
`"${args.path}" was not found in your node_modules. ` +
`Did you forget to install it?`,
args.path
);
}
}
return undefined;
});
},
} as esbuild.Plugin,
];

if (ctx.options.mode === "development" && ctx.config.future.unstable_dev) {
Expand Down
1 change: 0 additions & 1 deletion packages/remix-dev/compiler/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ type Mode = "development" | "production" | "test";
export type Options = {
mode: Mode;
sourcemap: boolean;
onWarning?: (message: string, key: string) => void;

// TODO: required in v2
devOrigin?: {
Expand Down
16 changes: 9 additions & 7 deletions packages/remix-dev/compiler/plugins/deprecatedRemixPackage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@ export function deprecatedRemixPackagePlugin(ctx: Context): Plugin {
// Warn on deprecated imports from the remix package
if (filePath === "remix") {
let relativePath = path.relative(process.cwd(), importer);
let warningMessage =
`WARNING: All \`remix\` exports are considered deprecated as of v1.3.3. ` +
`Please change your import in "${relativePath}" to come from the respective ` +
`underlying \`@remix-run/*\` package. ` +
`Run \`npx @remix-run/dev@latest codemod replace-remix-magic-imports\` ` +
`to automatically migrate your code.`;
ctx.options.onWarning?.(warningMessage, importer);
ctx.logger.warn(`deprecated \`remix\` import in ${relativePath}`, {
details: [
"Imports from the `remix` package were deprecated in v1.3.3.",
"Change your code to import from the appropriate `@remix-run/*` package instead.",
"You can run the following codemod to autofix this issue:",
"-> `npx @remix-run/dev@latest codemod replace-remix-magic-imports`",
],
key: importer,
});
}
return undefined;
});
Expand Down
Loading