From 225716c7fd31de04eadfef74c8eeee1e8b55923b Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Wed, 8 Mar 2023 14:27:12 -0800 Subject: [PATCH] fix: revert browserRouteModulesPlugin fix: put new browserRouteModulesPlugin behind `config.future.unstable_dev` --- .changeset/fix-x-route-imports.md | 5 + integration/shared-route-imports-test.ts | 60 +++++++ packages/remix-dev/compiler/compileBrowser.ts | 5 +- .../plugins/browserRouteModulesPlugin.ts | 134 ++++----------- .../plugins/browserRouteModulesPlugin_v2.ts | 153 ++++++++++++++++++ 5 files changed, 255 insertions(+), 102 deletions(-) create mode 100644 .changeset/fix-x-route-imports.md create mode 100644 integration/shared-route-imports-test.ts create mode 100644 packages/remix-dev/compiler/plugins/browserRouteModulesPlugin_v2.ts diff --git a/.changeset/fix-x-route-imports.md b/.changeset/fix-x-route-imports.md new file mode 100644 index 00000000000..71ed9591492 --- /dev/null +++ b/.changeset/fix-x-route-imports.md @@ -0,0 +1,5 @@ +--- +"@remix-run/dev": patch +--- + +fix issue with x-route imports creating multiple entries in the module graph diff --git a/integration/shared-route-imports-test.ts b/integration/shared-route-imports-test.ts new file mode 100644 index 00000000000..e6a0540fc66 --- /dev/null +++ b/integration/shared-route-imports-test.ts @@ -0,0 +1,60 @@ +import { test, expect } from "@playwright/test"; + +import { PlaywrightFixture } from "./helpers/playwright-fixture"; +import type { Fixture, AppFixture } from "./helpers/create-fixture"; +import { createAppFixture, createFixture, js } from "./helpers/create-fixture"; + +let fixture: Fixture; +let appFixture: AppFixture; + +test.beforeAll(async () => { + fixture = await createFixture({ + future: { v2_routeConvention: true }, + files: { + "app/routes/parent.jsx": js` + import { createContext, useContext } from "react"; + import { Outlet } from "@remix-run/react"; + + const ParentContext = createContext("❌"); + + export function useParentContext() { + return useContext(ParentContext); + } + + export default function Index() { + return ( + + + + ) + } + `, + + "app/routes/parent.child.jsx": js` + import { useParentContext } from "./parent"; + + export default function Index() { + return

{useParentContext()}

; + } + `, + }, + }); + + appFixture = await createAppFixture(fixture); +}); + +test.afterAll(() => { + appFixture.close(); +}); + +test("[description of what you expect it to do]", async ({ page }) => { + let app = new PlaywrightFixture(appFixture, page); + // If you need to test interactivity use the `app` + await app.goto("/parent/child", true); + + await page.waitForSelector("p:has-text('✅')"); +}); + +//////////////////////////////////////////////////////////////////////////////// +// 💿 Finally, push your changes to your fork of Remix and open a pull request! +//////////////////////////////////////////////////////////////////////////////// diff --git a/packages/remix-dev/compiler/compileBrowser.ts b/packages/remix-dev/compiler/compileBrowser.ts index eeb410fa5ce..128a611fcf1 100644 --- a/packages/remix-dev/compiler/compileBrowser.ts +++ b/packages/remix-dev/compiler/compileBrowser.ts @@ -14,6 +14,7 @@ import { getAppDependencies } from "./dependencies"; import { loaders } from "./loaders"; import type { CompileOptions } from "./options"; import { browserRouteModulesPlugin } from "./plugins/browserRouteModulesPlugin"; +import { browserRouteModulesPlugin as browserRouteModulesPlugin_v2 } from "./plugins/browserRouteModulesPlugin_v2"; import { cssFilePlugin } from "./plugins/cssFilePlugin"; import { deprecatedRemixPackagePlugin } from "./plugins/deprecatedRemixPackagePlugin"; import { emptyModulesPlugin } from "./plugins/emptyModulesPlugin"; @@ -122,7 +123,9 @@ const createEsbuildConfig = ( cssFilePlugin({ config, options }), urlImportsPlugin(), mdxPlugin(config), - browserRouteModulesPlugin(config, /\?browser$/, onLoader, mode), + config.future.unstable_dev + ? browserRouteModulesPlugin_v2(config, /\?browser$/, onLoader, mode) + : browserRouteModulesPlugin(config, /\?browser$/), emptyModulesPlugin(config, /\.server(\.[jt]sx?)?$/), NodeModulesPolyfillPlugin(), ].filter(isNotNull); diff --git a/packages/remix-dev/compiler/plugins/browserRouteModulesPlugin.ts b/packages/remix-dev/compiler/plugins/browserRouteModulesPlugin.ts index 5dd00f5197a..1ed616a290d 100644 --- a/packages/remix-dev/compiler/plugins/browserRouteModulesPlugin.ts +++ b/packages/remix-dev/compiler/plugins/browserRouteModulesPlugin.ts @@ -1,83 +1,28 @@ -import * as fs from "node:fs"; -import * as path from "node:path"; import type esbuild from "esbuild"; -import generate from "@babel/generator"; import type { RemixConfig } from "../../config"; -import invariant from "../../invariant"; -import * as Transform from "../../transform"; -import type { CompileOptions } from "../options"; -import { getLoaderForFile } from "../loaders"; import { getRouteModuleExports } from "../routeExports"; -import { applyHMR } from "./hmrPlugin"; - -const serverOnlyExports = new Set(["action", "loader"]); - -let removeServerExports = (onLoader: (loader: string) => void) => - Transform.create(({ types: t }) => { - return { - visitor: { - ExportNamedDeclaration: (path) => { - let { node } = path; - if (node.source) { - let specifiers = node.specifiers.filter(({ exported }) => { - let name = t.isIdentifier(exported) - ? exported.name - : exported.value; - return !serverOnlyExports.has(name); - }); - if (specifiers.length === node.specifiers.length) return; - if (specifiers.length === 0) return path.remove(); - path.replaceWith( - t.exportNamedDeclaration( - node.declaration, - specifiers, - node.source - ) - ); - } - if (t.isFunctionDeclaration(node.declaration)) { - let name = node.declaration.id?.name; - if (!name) return; - if (name === "loader") { - let { code } = generate(node); - onLoader(code); - } - if (serverOnlyExports.has(name)) return path.remove(); - } - if (t.isVariableDeclaration(node.declaration)) { - let declarations = node.declaration.declarations.filter((d) => { - let name = t.isIdentifier(d.id) ? d.id.name : undefined; - if (!name) return false; - if (name === "loader") { - let { code } = generate(node); - onLoader(code); - } - return !serverOnlyExports.has(name); - }); - if (declarations.length === 0) return path.remove(); - if (declarations.length === node.declaration.declarations.length) - return; - path.replaceWith( - t.variableDeclaration(node.declaration.kind, declarations) - ); - } - }, - }, - }; - }); +import invariant from "../../invariant"; type Route = RemixConfig["routes"][string]; +const browserSafeRouteExports: { [name: string]: boolean } = { + CatchBoundary: true, + ErrorBoundary: true, + default: true, + handle: true, + links: true, + meta: true, + shouldRevalidate: true, +}; + /** * This plugin loads route modules for the browser build, using module shims * that re-export only the route module exports that are safe for the browser. */ export function browserRouteModulesPlugin( config: RemixConfig, - suffixMatcher: RegExp, - onLoader: (filename: string, code: string) => void, - mode: CompileOptions["mode"] + suffixMatcher: RegExp ): esbuild.Plugin { return { name: "browser-route-modules", @@ -90,6 +35,7 @@ export function browserRouteModulesPlugin( }, new Map() ); + build.onResolve({ filter: suffixMatcher }, (args) => { return { path: args.path, @@ -100,54 +46,40 @@ export function browserRouteModulesPlugin( build.onLoad( { filter: suffixMatcher, namespace: "browser-route-module" }, async (args) => { + let theExports; let file = args.path.replace(suffixMatcher, ""); + let route = routesByFile.get(file); - if (/\.mdx?$/.test(file)) { - let route = routesByFile.get(file); + try { invariant(route, `Cannot get route by path: ${args.path}`); - let theExports = await getRouteModuleExports(config, route.id); - let contents = "module.exports = {};"; - if (theExports.length !== 0) { - let spec = `{ ${theExports.join(", ")} }`; - contents = `export ${spec} from ${JSON.stringify(`./${file}`)};`; - } - + theExports = (await getRouteModuleExports(config, route.id)).filter( + (ex) => !!browserSafeRouteExports[ex] + ); + } catch (error: any) { return { - contents, - resolveDir: config.appDirectory, - loader: "js", + errors: [ + { + text: error.message, + pluginName: "browser-route-module", + }, + ], }; } - let routeFile = path.join(config.appDirectory, file); - - let sourceCode = fs.readFileSync(routeFile, "utf8"); - - let transform = removeServerExports((loader: string) => - onLoader(routeFile, loader) - ); - let contents = transform(sourceCode, routeFile); - - if (mode === "development" && config.future.unstable_dev) { - contents = await applyHMR( - contents, - { - ...args, - path: routeFile, - }, - config, - !!build.initialOptions.sourcemap - ); + let contents = "module.exports = {};"; + if (theExports.length !== 0) { + let spec = `{ ${theExports.join(", ")} }`; + contents = `export ${spec} from ${JSON.stringify(`./${file}`)};`; } return { contents, - loader: getLoaderForFile(routeFile), - resolveDir: path.dirname(routeFile), + resolveDir: config.appDirectory, + loader: "js", }; } ); }, }; -} +} \ No newline at end of file diff --git a/packages/remix-dev/compiler/plugins/browserRouteModulesPlugin_v2.ts b/packages/remix-dev/compiler/plugins/browserRouteModulesPlugin_v2.ts new file mode 100644 index 00000000000..5dd00f5197a --- /dev/null +++ b/packages/remix-dev/compiler/plugins/browserRouteModulesPlugin_v2.ts @@ -0,0 +1,153 @@ +import * as fs from "node:fs"; +import * as path from "node:path"; +import type esbuild from "esbuild"; +import generate from "@babel/generator"; + +import type { RemixConfig } from "../../config"; +import invariant from "../../invariant"; +import * as Transform from "../../transform"; +import type { CompileOptions } from "../options"; +import { getLoaderForFile } from "../loaders"; +import { getRouteModuleExports } from "../routeExports"; +import { applyHMR } from "./hmrPlugin"; + +const serverOnlyExports = new Set(["action", "loader"]); + +let removeServerExports = (onLoader: (loader: string) => void) => + Transform.create(({ types: t }) => { + return { + visitor: { + ExportNamedDeclaration: (path) => { + let { node } = path; + if (node.source) { + let specifiers = node.specifiers.filter(({ exported }) => { + let name = t.isIdentifier(exported) + ? exported.name + : exported.value; + return !serverOnlyExports.has(name); + }); + if (specifiers.length === node.specifiers.length) return; + if (specifiers.length === 0) return path.remove(); + path.replaceWith( + t.exportNamedDeclaration( + node.declaration, + specifiers, + node.source + ) + ); + } + if (t.isFunctionDeclaration(node.declaration)) { + let name = node.declaration.id?.name; + if (!name) return; + if (name === "loader") { + let { code } = generate(node); + onLoader(code); + } + if (serverOnlyExports.has(name)) return path.remove(); + } + if (t.isVariableDeclaration(node.declaration)) { + let declarations = node.declaration.declarations.filter((d) => { + let name = t.isIdentifier(d.id) ? d.id.name : undefined; + if (!name) return false; + if (name === "loader") { + let { code } = generate(node); + onLoader(code); + } + return !serverOnlyExports.has(name); + }); + if (declarations.length === 0) return path.remove(); + if (declarations.length === node.declaration.declarations.length) + return; + path.replaceWith( + t.variableDeclaration(node.declaration.kind, declarations) + ); + } + }, + }, + }; + }); + +type Route = RemixConfig["routes"][string]; + +/** + * This plugin loads route modules for the browser build, using module shims + * that re-export only the route module exports that are safe for the browser. + */ +export function browserRouteModulesPlugin( + config: RemixConfig, + suffixMatcher: RegExp, + onLoader: (filename: string, code: string) => void, + mode: CompileOptions["mode"] +): esbuild.Plugin { + return { + name: "browser-route-modules", + async setup(build) { + let routesByFile: Map = Object.keys(config.routes).reduce( + (map, key) => { + let route = config.routes[key]; + map.set(route.file, route); + return map; + }, + new Map() + ); + build.onResolve({ filter: suffixMatcher }, (args) => { + return { + path: args.path, + namespace: "browser-route-module", + }; + }); + + build.onLoad( + { filter: suffixMatcher, namespace: "browser-route-module" }, + async (args) => { + let file = args.path.replace(suffixMatcher, ""); + + if (/\.mdx?$/.test(file)) { + let route = routesByFile.get(file); + invariant(route, `Cannot get route by path: ${args.path}`); + + let theExports = await getRouteModuleExports(config, route.id); + let contents = "module.exports = {};"; + if (theExports.length !== 0) { + let spec = `{ ${theExports.join(", ")} }`; + contents = `export ${spec} from ${JSON.stringify(`./${file}`)};`; + } + + return { + contents, + resolveDir: config.appDirectory, + loader: "js", + }; + } + + let routeFile = path.join(config.appDirectory, file); + + let sourceCode = fs.readFileSync(routeFile, "utf8"); + + let transform = removeServerExports((loader: string) => + onLoader(routeFile, loader) + ); + let contents = transform(sourceCode, routeFile); + + if (mode === "development" && config.future.unstable_dev) { + contents = await applyHMR( + contents, + { + ...args, + path: routeFile, + }, + config, + !!build.initialOptions.sourcemap + ); + } + + return { + contents, + loader: getLoaderForFile(routeFile), + resolveDir: path.dirname(routeFile), + }; + } + ); + }, + }; +}