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

Clean up types within consolidated package #12177

Merged
merged 6 commits into from
Oct 23, 2024
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
11 changes: 11 additions & 0 deletions .changeset/sour-cycles-lie.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
"@react-router/dev": major
"react-router": major
---

- Consolidate types previously duplicated across `@remix-run/router`, `@remix-run/server-runtime`, and `@remix-run/react` now that they all live in `react-router`
- Examples: `LoaderFunction`, `LoaderFunctionArgs`, `ActionFunction`, `ActionFunctionArgs`, `DataFunctionArgs`, `RouteManifest`, `LinksFunction`, `Route`, `EntryRoute`
- The `RouteManifest` type used by the "remix" code is now slightly stricter because it is using the former `@remix-run/router` `RouteManifest`
- `Record<string, Route> -> Record<string, Route | undefined>`
- Removed `AppData` type in favor of inlining `unknown` in the few locations it was used
- Removed `ServerRuntimeMeta*` types in favor of the `Meta*` types they were duplicated from
10 changes: 6 additions & 4 deletions packages/react-router-dev/vite/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2023,11 +2023,13 @@ function groupRoutesByParentId(manifest: ServerBuild["routes"]) {
let routes: Record<string, Omit<ServerRoute, "children">[]> = {};

Object.values(manifest).forEach((route) => {
let parentId = route.parentId || "";
if (!routes[parentId]) {
routes[parentId] = [];
if (route) {
let parentId = route.parentId || "";
if (!routes[parentId]) {
routes[parentId] = [];
}
routes[parentId].push(route);
}
routes[parentId].push(route);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Remix RouteManifest was Record<string, Route>
  • React Router's internal RouteManifest was more accurately Record<string, Route | undefined>

Switching to the stricter type means we have to introduce some defensiveness in existing Remix code - which feels more appropriate given the new fog of war behavior

});

return routes;
Expand Down
19 changes: 9 additions & 10 deletions packages/react-router-dev/vite/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,14 +171,16 @@ const findDeps = async (
};

const groupRoutesByParentId = (manifest: ServerRouteManifest) => {
let routes: Record<string, Omit<ServerRoute, "children">[]> = {};
let routes: Record<string, NonNullable<ServerRoute>[]> = {};

Object.values(manifest).forEach((route) => {
let parentId = route.parentId || "";
if (!routes[parentId]) {
routes[parentId] = [];
if (route) {
let parentId = route.parentId || "";
if (!routes[parentId]) {
routes[parentId] = [];
}
routes[parentId].push(route);
}
routes[parentId].push(route);
});

return routes;
Expand All @@ -189,11 +191,8 @@ const groupRoutesByParentId = (manifest: ServerRouteManifest) => {
const createRoutes = (
manifest: ServerRouteManifest,
parentId: string = "",
routesByParentId: Record<
string,
Omit<ServerRoute, "children">[]
> = groupRoutesByParentId(manifest)
): ServerRoute[] => {
routesByParentId = groupRoutesByParentId(manifest)
): NonNullable<ServerRoute>[] => {
return (routesByParentId[parentId] || []).map((route) => ({
...route,
children: createRoutes(manifest, route.id, routesByParentId),
Expand Down
16 changes: 0 additions & 16 deletions packages/react-router/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -213,12 +213,6 @@ export { createRoutesStub } from "./lib/dom/ssr/routes-test-stub";
// Expose old @remix-run/server-runtime API, minus duplicate APIs
export { createCookie, isCookie } from "./lib/server-runtime/cookies";

// TODO: (v7) Clean up code paths for these exports
// export {
// json,
// redirect,
// redirectDocument,
// } from "./lib/server-runtime/responses";
export { createRequestHandler } from "./lib/server-runtime/server";
export {
createSession,
Expand Down Expand Up @@ -258,16 +252,6 @@ export type {
} from "./lib/router/links";

export type {
// TODO: (v7) Clean up code paths for these exports
// ActionFunction,
// ActionFunctionArgs,
// LinksFunction,
// LoaderFunction,
// LoaderFunctionArgs,
// ServerRuntimeMetaArgs,
// ServerRuntimeMetaDescriptor,
// ServerRuntimeMetaFunction,
DataFunctionArgs,
HeadersArgs,
HeadersFunction,
} from "./lib/server-runtime/routeModules";
Expand Down
1 change: 1 addition & 0 deletions packages/react-router/lib/dom-export/hydrated-router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ function createHydratedRouter(): DataRouter {
// * or doesn't have a server loader and we have no data to render
if (
route &&
manifestRoute &&
shouldHydrateRouteLoader(
manifestRoute,
route,
Expand Down
9 changes: 5 additions & 4 deletions packages/react-router/lib/dom/ssr/components.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,8 @@ function PrefetchPageLinksImpl({
let routesParams = new Set<string>();
let foundOptOutRoute = false;
nextMatches.forEach((m) => {
if (!manifest.routes[m.route.id].hasLoader) {
let manifestRoute = manifest.routes[m.route.id];
if (!manifestRoute || !manifestRoute.hasLoader) {
return;
}

Expand All @@ -376,7 +377,7 @@ function PrefetchPageLinksImpl({
routeModules[m.route.id]?.shouldRevalidate
) {
foundOptOutRoute = true;
} else if (manifest.routes[m.route.id].hasClientLoader) {
} else if (manifestRoute.hasClientLoader) {
foundOptOutRoute = true;
} else {
routesParams.add(m.route.id);
Expand Down Expand Up @@ -682,7 +683,7 @@ ${matches
.map(
(match, index) =>
`import * as route${index} from ${JSON.stringify(
manifest.routes[match.route.id].module
manifest.routes[match.route.id]!.module
)};`
)
.join("\n")}
Expand Down Expand Up @@ -728,7 +729,7 @@ import(${JSON.stringify(manifest.entry.module)});`;
let routePreloads = matches
.map((match) => {
let route = manifest.routes[match.route.id];
return (route.imports || []).concat([route.module]);
return route ? (route.imports || []).concat([route.module]) : [];
})
.flat(1);

Expand Down
5 changes: 0 additions & 5 deletions packages/react-router/lib/dom/ssr/data.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
import "../global";

/**
* Data for a route that was returned from a `loader()`.
*/
export type AppData = unknown;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got rid of this - just used unknown in the few spots this was used


export async function createRequestInit(
request: Request
): Promise<RequestInit> {
Expand Down
7 changes: 4 additions & 3 deletions packages/react-router/lib/dom/ssr/entry.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import type { StaticHandlerContext } from "../../router/router";

import type { RouteManifest, EntryRoute } from "./routes";
import type { EntryRoute } from "./routes";
import type { RouteModules } from "./routeModules";

// Object passed to RemixContext.Provider
import type { RouteManifest } from "../../router/utils";

type SerializedError = {
message: string;
stack?: string;
};

// Object passed to RemixContext.Provider
export interface FrameworkContextObject {
manifest: AssetsManifest;
routeModules: RouteModules;
Expand Down
17 changes: 9 additions & 8 deletions packages/react-router/lib/dom/ssr/fog-of-war.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import * as React from "react";
import type { PatchRoutesOnNavigationFunction } from "../../context";
import type { Router as DataRouter } from "../../router/router";
import type { RouteManifest } from "../../router/utils";
import { matchRoutes } from "../../router/utils";
import type { AssetsManifest } from "./entry";
import type { RouteModules } from "./routeModules";
import type { EntryRoute } from "./routes";
import { createClientRoutes } from "./routes";

declare global {
Expand Down Expand Up @@ -233,13 +235,12 @@ export async function fetchAndApplyManifestPatches(

// Patch routes we don't know about yet into the manifest
let knownRoutes = new Set(Object.keys(manifest.routes));
let patches: AssetsManifest["routes"] = Object.values(serverPatches).reduce(
(acc, route) =>
!knownRoutes.has(route.id)
? Object.assign(acc, { [route.id]: route })
: acc,
{}
);
let patches = Object.values(serverPatches).reduce((acc, route) => {
if (route && !knownRoutes.has(route.id)) {
acc[route.id] = route;
}
return acc;
}, {} as RouteManifest<EntryRoute>);
Object.assign(manifest.routes, patches);

// Track discovered paths so we don't have to fetch them again
Expand All @@ -249,7 +250,7 @@ export async function fetchAndApplyManifestPatches(
// in their new children
let parentIds = new Set<string | undefined>();
Object.values(patches).forEach((patch) => {
if (!patch.parentId || !patches[patch.parentId]) {
if (patch && (!patch.parentId || !patches[patch.parentId])) {
parentIds.add(patch.parentId);
}
});
Expand Down
21 changes: 12 additions & 9 deletions packages/react-router/lib/dom/ssr/links.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ export function getKeyedLinksForMatches(
let module = routeModules[match.route.id];
let route = manifest.routes[match.route.id];
return [
route.css ? route.css.map((href) => ({ rel: "stylesheet", href })) : [],
route && route.css
? route.css.map((href) => ({ rel: "stylesheet", href }))
: [],
module?.links?.() || [],
];
})
Expand Down Expand Up @@ -138,11 +140,12 @@ export async function getKeyedPrefetchLinks(
): Promise<KeyedHtmlLinkDescriptor[]> {
let links = await Promise.all(
matches.map(async (match) => {
let mod = await loadRouteModule(
manifest.routes[match.route.id],
routeModules
);
return mod.links ? mod.links() : [];
let route = manifest.routes[match.route.id];
if (route) {
let mod = await loadRouteModule(route, routeModules);
return mod.links ? mod.links() : [];
}
return [];
})
);

Expand Down Expand Up @@ -197,7 +200,7 @@ export function getNewMatchesForLinks(
if (mode === "data") {
return nextMatches.filter((match, index) => {
let manifestRoute = manifest.routes[match.route.id];
if (!manifestRoute.hasLoader) {
if (!manifestRoute || !manifestRoute.hasLoader) {
return false;
}

Expand Down Expand Up @@ -235,6 +238,7 @@ export function getModuleLinkHrefs(
matches
.map((match) => {
let route = manifestPatch.routes[match.route.id];
if (!route) return [];
let hrefs = [route.module];
if (route.imports) {
hrefs = hrefs.concat(route.imports);
Expand All @@ -256,12 +260,11 @@ function getCurrentPageModulePreloadHrefs(
matches
.map((match) => {
let route = manifest.routes[match.route.id];
if (!route) return [];
let hrefs = [route.module];

if (route.imports) {
hrefs = hrefs.concat(route.imports);
}

return hrefs;
})
.flat(1)
Expand Down
Loading