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

Lift static handler creation and pass context through query/queryRoute #4790

Merged
merged 2 commits into from Dec 7, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/silver-fireants-grab.md
@@ -0,0 +1,5 @@
---
"@remix-run/server-runtime": patch
---

Fix performance regression with ctreation of `@remix-run/router` static handler
2 changes: 1 addition & 1 deletion packages/remix-server-runtime/package.json
Expand Up @@ -16,7 +16,7 @@
"typings": "dist/index.d.ts",
"module": "dist/esm/index.js",
"dependencies": {
"@remix-run/router": "1.0.5-pre.1",
"@remix-run/router": "1.0.5-pre.2",
"@types/cookie": "^0.4.0",
"@web3-storage/multipart-parser": "^1.0.0",
"cookie": "^0.4.1",
Expand Down
19 changes: 7 additions & 12 deletions packages/remix-server-runtime/routes.ts
@@ -1,11 +1,9 @@
// TODO: RRR - Change import to @remix-run/router
import type {
AgnosticDataRouteObject,
ActionFunctionArgs,
LoaderFunctionArgs,
} from "@remix-run/router";

import { type AppLoadContext } from "./data";
import { callRouteActionRR, callRouteLoaderRR } from "./data";
import type { ServerRouteModule } from "./routeModules";

Expand Down Expand Up @@ -56,7 +54,6 @@ export function createRoutes(
// createStaticHandler
export function createStaticHandlerDataRoutes(
manifest: ServerRouteManifest,
loadContext: AppLoadContext,
parentId?: string
): AgnosticDataRouteObject[] {
return Object.values(manifest)
Expand All @@ -73,17 +70,19 @@ export function createStaticHandlerDataRoutes(
loader: route.module.loader
? (args: LoaderFunctionArgs) =>
callRouteLoaderRR({
...args,
loadContext,
request: args.request,
params: args.params,
loadContext: args.context,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer comes from the closure scope and instead passes straight through

loader: route.module.loader!,
routeId: route.id,
})
: undefined,
action: route.module.action
? (args: ActionFunctionArgs) =>
callRouteActionRR({
...args,
loadContext,
request: args.request,
params: args.params,
loadContext: args.context,
action: route.module.action!,
routeId: route.id,
})
Expand All @@ -100,11 +99,7 @@ export function createStaticHandlerDataRoutes(
}
: {
caseSensitive: route.caseSensitive,
children: createStaticHandlerDataRoutes(
manifest,
loadContext,
route.id
),
children: createStaticHandlerDataRoutes(manifest, route.id),
...commonRoute,
};
});
Expand Down
44 changes: 28 additions & 16 deletions packages/remix-server-runtime/server.ts
Expand Up @@ -35,25 +35,24 @@ export const createRequestHandler: CreateRequestHandlerFunction = (
mode
) => {
let routes = createRoutes(build.routes);
let dataRoutes = createStaticHandlerDataRoutes(build.routes);
let serverMode = isServerMode(mode) ? mode : ServerMode.Production;
let staticHandler = unstable_createStaticHandler(dataRoutes);

return async function requestHandler(request, loadContext = {}) {
let url = new URL(request.url);
let matches = matchServerRoutes(routes, url.pathname);

let staticHandler = unstable_createStaticHandler(
createStaticHandlerDataRoutes(build.routes, loadContext)
);

let response: Response;
if (url.searchParams.has("_data")) {
let routeId = url.searchParams.get("_data")!;

response = await handleDataRequestRR(
serverMode,
staticHandler!,
staticHandler,
routeId,
request
request,
loadContext
);

if (build.entry.module.handleDataRequest) {
Expand All @@ -70,16 +69,18 @@ export const createRequestHandler: CreateRequestHandlerFunction = (
) {
response = await handleResourceRequestRR(
serverMode,
staticHandler!,
staticHandler,
matches.slice(-1)[0].route.id,
request
request,
loadContext
);
} else {
response = await handleDocumentRequestRR(
serverMode,
build,
staticHandler!,
request
staticHandler,
request,
loadContext
);
}

Expand All @@ -99,10 +100,14 @@ async function handleDataRequestRR(
serverMode: ServerMode,
staticHandler: StaticHandler,
routeId: string,
request: Request
request: Request,
loadContext: AppLoadContext
) {
try {
let response = await staticHandler.queryRoute(request, routeId);
let response = await staticHandler.queryRoute(request, {
routeId,
requestContext: loadContext,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Send the context through query/queryRoute


if (isRedirectResponse(response)) {
// We don't have any way to prevent a fetch request from following
Expand Down Expand Up @@ -204,11 +209,14 @@ async function handleDocumentRequestRR(
serverMode: ServerMode,
build: ServerBuild,
staticHandler: StaticHandler,
request: Request
request: Request,
loadContext: AppLoadContext
) {
let context;
try {
context = await staticHandler.query(request);
context = await staticHandler.query(request, {
requestContext: loadContext,
});
} catch (error) {
if (!request.signal.aborted && serverMode !== ServerMode.Test) {
console.error(error);
Expand Down Expand Up @@ -364,13 +372,17 @@ async function handleResourceRequestRR(
serverMode: ServerMode,
staticHandler: StaticHandler,
routeId: string,
request: Request
request: Request,
loadContext: AppLoadContext
) {
try {
// Note we keep the routeId here to align with the Remix handling of
// resource routes which doesn't take ?index into account and just takes
// the leaf match
let response = await staticHandler.queryRoute(request, routeId);
let response = await staticHandler.queryRoute(request, {
routeId,
requestContext: loadContext,
});
// callRouteLoader/callRouteAction always return responses
invariant(
isResponse(response),
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
Expand Up @@ -2069,10 +2069,10 @@
"@changesets/types" "^5.0.0"
dotenv "^8.1.0"

"@remix-run/router@1.0.5-pre.1":
version "1.0.5-pre.1"
resolved "https://registry.npmjs.org/@remix-run/router/-/router-1.0.5-pre.1.tgz#dd0939d1631ebd56faa2bc69141e94a96887c7e5"
integrity sha512-dDfVwp0ta99+7vMKMDb5eh1Pc9CWZ5/Q+679YojyHyrZqSWJmlZyCP7prs6UAaXaGg5MveP1pCav/YFCINlcFw==
"@remix-run/router@1.0.5-pre.2":
version "1.0.5-pre.2"
resolved "https://registry.npmjs.org/@remix-run/router/-/router-1.0.5-pre.2.tgz#63a79484e70ac7b782dfbea011ece1f65158f3c1"
integrity sha512-5dD9v6wogaH2VbGsc74ua89pSAlsHmzhxgM/A8kpZXVd7ym33zWYBTxhqpaFzMWsMmvIEpvf7diJb3ACadyRnQ==

"@remix-run/web-blob@^3.0.3", "@remix-run/web-blob@^3.0.4":
version "3.0.4"
Expand Down