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(remix): Add dynamic loading of @remix-run/router module. #10458

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/remix/src/utils/instrumentServer.ts
Expand Up @@ -29,6 +29,7 @@ import {

import { DEBUG_BUILD } from './debug-build';
import { getFutureFlagsServer, getRemixVersionFromBuild } from './futureFlags';
import { loadRemixRouterModule } from './routerLoader';
import {
extractData,
getRequestMatch,
Expand Down Expand Up @@ -59,6 +60,8 @@ import { normalizeRemixRequest } from './web-fetch';
let FUTURE_FLAGS: FutureConfig | undefined;
let IS_REMIX_V2: boolean | undefined;

let pkg: ReactRouterDomPkg | undefined;

const redirectStatusCodes = new Set([301, 302, 303, 307, 308]);
function isRedirectResponse(response: Response): boolean {
return redirectStatusCodes.has(response.status);
Expand Down Expand Up @@ -447,7 +450,6 @@ export function getTransactionName(

function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBuild): RequestHandler {
const routes = createRoutes(build.routes);
const pkg = loadModule<ReactRouterDomPkg>('react-router-dom');

return async function (this: unknown, request: RemixRequest, loadContext?: AppLoadContext): Promise<Response> {
// This means that the request handler of the adapter (ex: express) is already wrapped.
Expand All @@ -456,6 +458,10 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui
return origRequestHandler.call(this, request, loadContext);
}

if (!pkg) {
pkg = await loadRemixRouterModule();
}

return runWithAsyncContext(async () => {
// eslint-disable-next-line deprecation/deprecation
const hub = getCurrentHub();
Expand Down
51 changes: 51 additions & 0 deletions packages/remix/src/utils/routerLoader.ts
@@ -0,0 +1,51 @@
import { loadModule, logger } from '@sentry/utils';
import { cwd } from 'process';
import { DEBUG_BUILD } from './debug-build';
import type { ReactRouterDomPkg } from './vendor/types';

function hasMatchRoutes(pkg: ReactRouterDomPkg | undefined): boolean {
return !!pkg && typeof pkg.matchRoutes === 'function';
}

/**
* Loads the router package that provides matchRoutes.
* Tries loading @remix-run/router first, then falls back to react-router-dom.
*/
export async function loadRemixRouterModule(): Promise<ReactRouterDomPkg | undefined> {
// Try loading @remix-run/router first, then fall back to react-router-dom
for (const moduleName of ['@remix-run/router', 'react-router-dom']) {
const pkg = await tryLoadRouterModule(moduleName);

if (pkg) {
return pkg;
}
}

DEBUG_BUILD && logger.warn('Could not find a router package that provides `matchRoutes`.');

return;
}

async function tryLoadRouterModule(moduleName: string): Promise<ReactRouterDomPkg | undefined> {
let pkg: ReactRouterDomPkg | undefined;

pkg = loadModule<ReactRouterDomPkg>(moduleName);

if (hasMatchRoutes(pkg)) {
return pkg;
}

try {
pkg = await import(moduleName);
} catch (e) {
pkg = await import(`${cwd()}/node_modules/${moduleName}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't work in CJS right? Can you confirm that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have done a little research, but could not produce a case where this breaks. It looks like it works fine on both CJS and ESM. Not in worker environments though.

But, while checking the previous discussions (#5897), the main reason we need this was our TS version and it's up to date now, so we can just try making @remix-run/router a dependency and stop doing this anyways? WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we can just try making @remix-run/router a dependency and stop doing this anyways? WDYT

Let's do that instead!! Less dynamic imports the better.

}

if (hasMatchRoutes(pkg)) {
return pkg;
} else {
DEBUG_BUILD && logger.warn(`Could not find ${moduleName} package.`);
}

return;
}
14 changes: 3 additions & 11 deletions packages/remix/src/utils/serverAdapters/express.ts
Expand Up @@ -9,10 +9,10 @@ import {
import { flush } from '@sentry/node';
import type { Transaction } from '@sentry/types';
import { extractRequestData, fill, isString, logger } from '@sentry/utils';
import { cwd } from 'process';

import { DEBUG_BUILD } from '../debug-build';
import { createRoutes, getTransactionName, instrumentBuild, startRequestHandlerTransaction } from '../instrumentServer';
import { loadRemixRouterModule } from '../routerLoader';
import type {
AppLoadContext,
ExpressCreateRequestHandler,
Expand All @@ -26,7 +26,7 @@ import type {
ServerBuild,
} from '../vendor/types';

let pkg: ReactRouterDomPkg;
let pkg: ReactRouterDomPkg | undefined;

function wrapExpressRequestHandler(
origRequestHandler: ExpressRequestHandler,
Expand All @@ -41,15 +41,7 @@ function wrapExpressRequestHandler(
next: ExpressNextFunction,
): Promise<void> {
if (!pkg) {
try {
pkg = await import('react-router-dom');
} catch (e) {
pkg = await import(`${cwd()}/node_modules/react-router-dom`);
} finally {
if (!pkg) {
DEBUG_BUILD && logger.error('Could not find `react-router-dom` package.');
}
}
pkg = await loadRemixRouterModule();
}

await runWithAsyncContext(async () => {
Expand Down
10 changes: 5 additions & 5 deletions packages/remix/test/integration/package.json
Expand Up @@ -7,16 +7,16 @@
"start": "remix-serve build"
},
"dependencies": {
"@remix-run/express": "1.17.0",
"@remix-run/node": "1.17.0",
"@remix-run/react": "1.17.0",
"@remix-run/serve": "1.17.0",
"@remix-run/express": "1.19.3",
"@remix-run/node": "1.19.3",
"@remix-run/react": "1.19.3",
"@remix-run/serve": "1.19.3",
"@sentry/remix": "file:../..",
"react": "^17.0.2",
"react-dom": "^17.0.2"
},
"devDependencies": {
"@remix-run/dev": "1.17.0",
"@remix-run/dev": "1.19.3",
"@types/react": "^17.0.47",
"@types/react-dom": "^17.0.17",
"nock": "^13.1.0",
Expand Down