From 22364f44c5cd9dfbaa925ccaa4667b5cd5daaefa Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Fri, 21 Jul 2023 12:57:12 -0400 Subject: [PATCH] fix(dev): remove outdated esm import warnings (#6916) --- .changeset/selfish-months-behave.md | 8 + integration/esm-only-warning-test.ts | 189 ------------------ .../compiler/server/plugins/bareImports.ts | 101 +--------- 3 files changed, 10 insertions(+), 288 deletions(-) create mode 100644 .changeset/selfish-months-behave.md delete mode 100644 integration/esm-only-warning-test.ts diff --git a/.changeset/selfish-months-behave.md b/.changeset/selfish-months-behave.md new file mode 100644 index 00000000000..f09d4359637 --- /dev/null +++ b/.changeset/selfish-months-behave.md @@ -0,0 +1,8 @@ +--- +"@remix-run/dev": patch +--- + +Remove outdated ESM import warnings + +Most of the time these warnings were false positives. +Instead, we now rely on built-in Node warnings for ESM imports. diff --git a/integration/esm-only-warning-test.ts b/integration/esm-only-warning-test.ts deleted file mode 100644 index c920af4f281..00000000000 --- a/integration/esm-only-warning-test.ts +++ /dev/null @@ -1,189 +0,0 @@ -import { test, expect } from "@playwright/test"; -import { PassThrough } from "stream"; - -import { createFixtureProject, js, json } from "./helpers/create-fixture"; - -let buildOutput: string; - -test.beforeAll(async () => { - let buildStdio = new PassThrough(); - - await createFixtureProject({ - buildStdio, - config: { - future: { v2_routeConvention: true }, - }, - files: { - "package.json": json({ - name: "remix-integration-9v4bpv66vd", - private: true, - sideEffects: false, - scripts: { - build: "remix build", - dev: "remix dev", - start: "remix-serve build", - }, - dependencies: { - "@remix-run/node": "0.0.0-local-version", - "@remix-run/react": "0.0.0-local-version", - "@remix-run/serve": "0.0.0-local-version", - isbot: "0.0.0-local-version", - react: "0.0.0-local-version", - "react-dom": "0.0.0-local-version", - "esm-only-no-exports": "0.0.0-local-version", - "esm-only-exports": "0.0.0-local-version", - "esm-only-sub-exports": "0.0.0-local-version", - "esm-cjs-exports": "0.0.0-local-version", - }, - devDependencies: { - "@remix-run/dev": "0.0.0-local-version", - }, - }), - "app/routes/_index.jsx": js` - import { json } from "@remix-run/node"; - import { Link, useLoaderData } from "@remix-run/react"; - import a from "esm-only-no-exports"; - import b from "esm-only-exports"; - import c from "esm-only-sub-exports"; - import d from "esm-cjs-exports"; - import e from "cjs-dynamic-import"; - - export async function loader() { - let { default: f } = await import("esm-only-exports-b"); - return json({ - a: a(), - b: b(), - c: c(), - d: d(), - e: e(), - f: f(), - }); - } - - export default function Index() { - let data = useLoaderData(); - return null; - } - `, - "node_modules/esm-only-no-exports/package.json": json({ - name: "esm-only-no-exports", - version: "1.0.0", - type: "module", - main: "index.js", - }), - "node_modules/esm-only-no-exports/index.js": js` - export default () => "esm-only-no-exports"; - `, - "node_modules/esm-only-exports/package.json": json({ - name: "esm-only-exports", - version: "1.0.0", - type: "module", - main: "index.js", - exports: { - ".": "./index.js", - "./package.json": "./package.json", - }, - }), - "node_modules/esm-only-exports/index.js": js` - export default () => "esm-only-no-exports"; - `, - "node_modules/esm-only-exports-b/package.json": json({ - name: "esm-only-exports-b", - version: "1.0.0", - type: "module", - main: "index.js", - exports: { - ".": "./index.js", - "./package.json": "./package.json", - }, - }), - "node_modules/esm-only-exports-b/index.js": js` - export default () => "esm-only-no-exports-b"; - `, - "node_modules/esm-only-exports-c/package.json": json({ - name: "esm-only-exports-c", - version: "1.0.0", - type: "module", - main: "index.js", - exports: { - ".": "./index.js", - "./package.json": "./package.json", - }, - }), - "node_modules/esm-only-exports-c/index.js": js` - export default () => "esm-only-no-exports-c"; - `, - "node_modules/cjs-dynamic-import/package.json": json({ - name: "cjs-dynamic-import", - version: "1.0.0", - main: "index.js", - }), - "node_modules/cjs-dynamic-import/index.js": js` - module.exports = async () => "esm-only-no-exports-d" + (await import("esm-only-exports-c")).default(); - `, - "node_modules/esm-only-sub-exports/package.json": json({ - name: "esm-only-sub-exports", - version: "1.0.0", - type: "module", - main: "index.js", - exports: { - ".": "./index.js", - "./sub": "./sub.js", - "./package.json": "./package.json", - }, - }), - "node_modules/esm-only-sub-exports/index.js": js` - export default () => "esm-only-no-exports"; - `, - "node_modules/esm-only-sub-exports/sub.js": js` - export default () => "esm-only-no-exports/sub"; - `, - "node_modules/esm-cjs-exports/package.json": json({ - name: "esm-cjs-exports", - version: "1.0.0", - type: "module", - main: "index.js", - exports: { - ".": { - require: "./index.cjs", - default: "./index.js", - }, - "./sub": { - require: "./sub.cjs", - default: "./sub.js", - }, - "./package.json": "./package.json", - }, - }), - "node_modules/esm-cjs-exports/index.js": js` - export default () => "esm-only-no-exports"; - `, - "node_modules/esm-cjs-exports/index.cjs": js` - module.exports = () => "esm-only-no-exports"; - `, - "node_modules/esm-cjs-exports/sub.js": js` - export default () => "esm-only-no-exports/sub"; - `, - "node_modules/esm-cjs-exports/sub.cjs": js` - module.exports = () => "esm-only-no-exports/sub"; - `, - }, - }); - - let chunks: Buffer[] = []; - buildOutput = await new Promise((resolve, reject) => { - buildStdio.on("data", (chunk) => chunks.push(Buffer.from(chunk))); - buildStdio.on("error", (err) => reject(err)); - buildStdio.on("end", () => resolve(Buffer.concat(chunks).toString("utf8"))); - }); -}); - -test("logs warnings for ESM only packages", async () => { - 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"); -}); diff --git a/packages/remix-dev/compiler/server/plugins/bareImports.ts b/packages/remix-dev/compiler/server/plugins/bareImports.ts index 75c95a1d45b..211a816d988 100644 --- a/packages/remix-dev/compiler/server/plugins/bareImports.ts +++ b/packages/remix-dev/compiler/server/plugins/bareImports.ts @@ -1,5 +1,4 @@ -import path, { isAbsolute, relative } from "path"; -import fs from "fs"; +import { isAbsolute, relative } from "path"; import { builtinModules } from "module"; import type { Plugin } from "esbuild"; @@ -34,7 +33,7 @@ export function serverBareModulesPlugin(ctx: Context): Plugin { return { name: "server-bare-modules", setup(build) { - build.onResolve({ filter: /.*/ }, ({ importer, kind, path }) => { + build.onResolve({ filter: /.*/ }, ({ importer, path }) => { // If it's not a bare module ID, bundle it. if (!isBareModuleId(resolvePath(path))) { return undefined; @@ -113,14 +112,6 @@ export function serverBareModulesPlugin(ctx: Context): Plugin { } } - if ( - !isNodeBuiltIn(packageName) && - kind !== "dynamic-import" && - ctx.config.serverPlatform === "node" - ) { - warnOnceIfEsmOnlyPackage(ctx, packageName, path, importer); - } - // Externalize everything else if we've gotten here. return { path, @@ -145,91 +136,3 @@ function getNpmPackageName(id: string): string { function isBareModuleId(id: string): boolean { return !id.startsWith("node:") && !id.startsWith(".") && !isAbsolute(id); } - -function warnOnceIfEsmOnlyPackage( - ctx: Context, - packageName: string, - fullImportPath: string, - importer: string -) { - try { - let packageDir = resolveModuleBasePath( - packageName, - fullImportPath, - importer - ); - let packageJsonFile = path.join(packageDir, "package.json"); - - if (!fs.existsSync(packageJsonFile)) { - ctx.logger.warn(`could not find package.json for ${packageName}`); - return; - } - let pkg = JSON.parse(fs.readFileSync(packageJsonFile, "utf-8")); - - let subImport = fullImportPath.slice(packageName.length + 1); - - if (pkg.type === "module") { - let isEsmOnly = true; - if (pkg.exports) { - if (!subImport) { - if (pkg.exports.require) { - isEsmOnly = false; - } else if (pkg.exports["."]?.require) { - isEsmOnly = false; - } - } else if (pkg.exports[`./${subImport}`]?.require) { - isEsmOnly = false; - } - } - - if (isEsmOnly) { - ctx.logger.warn(`esm-only package: ${packageName}`, { - details: [ - `${packageName} is possibly an ESM-only package.`, - "To bundle it with your server, include it in `serverDependenciesToBundle`", - "-> https://remix.run/docs/en/main/file-conventions/remix-config#serverdependenciestobundle", - ], - key: packageName + ":esm-only", - }); - } - } - } catch (error: unknown) { - // module not installed - // we warned earlier if a package is used without being in package.json - // if the build fails, the reason will be right there - } -} - -// https://github.com/nodejs/node/issues/33460#issuecomment-919184789 -// adapted to use the fullImportPath to resolve sub packages like @heroicons/react/solid -function resolveModuleBasePath( - packageName: string, - fullImportPath: string, - importer: string -) { - let moduleMainFilePath = require.resolve(fullImportPath, { - paths: [importer], - }); - - let packageNameParts = packageName.split("/"); - - let searchForPathSection; - - if (packageName.startsWith("@") && packageNameParts.length > 1) { - let [org, mod] = packageNameParts; - searchForPathSection = `node_modules${path.sep}${org}${path.sep}${mod}`; - } else { - let [mod] = packageNameParts; - searchForPathSection = `node_modules${path.sep}${mod}`; - } - - let lastIndex = moduleMainFilePath.lastIndexOf(searchForPathSection); - - if (lastIndex === -1) { - throw new Error( - `Couldn't resolve the base path of "${packageName}". Searched inside the resolved main file path "${moduleMainFilePath}" using "${searchForPathSection}"` - ); - } - - return moduleMainFilePath.slice(0, lastIndex + searchForPathSection.length); -}