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

fix: revert browserRouteModulesPlugin #5721

Merged
merged 1 commit into from
Mar 9, 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/fix-x-route-imports.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/dev": patch
---

fix issue with x-route imports creating multiple entries in the module graph
60 changes: 60 additions & 0 deletions integration/shared-route-imports-test.ts
Original file line number Diff line number Diff line change
@@ -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 (
<ParentContext.Provider value="✅">
<Outlet />
</ParentContext.Provider>
)
}
`,

"app/routes/parent.child.jsx": js`
import { useParentContext } from "./parent";

export default function Index() {
return <p>{useParentContext()}</p>;
}
`,
},
});

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!
////////////////////////////////////////////////////////////////////////////////
5 changes: 4 additions & 1 deletion packages/remix-dev/compiler/compileBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
Expand Down
134 changes: 33 additions & 101 deletions packages/remix-dev/compiler/plugins/browserRouteModulesPlugin.ts
Original file line number Diff line number Diff line change
@@ -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",
Expand All @@ -90,6 +35,7 @@ export function browserRouteModulesPlugin(
},
new Map()
);

build.onResolve({ filter: suffixMatcher }, (args) => {
return {
path: args.path,
Expand All @@ -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",
};
}
);
},
};
}
}
Loading