ronantakizawa/codereview-bench huggingface dataset:
Repository: remix-run/react-router
File: packages/react-router/tsup.config.ts (TypeScript)

Code before:
import { defineConfig } from "tsup";

// @ts-ignore - out of scope
import { createBanner } from "../../build.utils.js";

import pkg from "./package.json";

const entry = ["index.ts", "dom-export.ts", "lib/types/route-module.ts"];

const config = (enableDevWarnings: boolean) =>
  defineConfig([
    {
      clean: false,
      entry,
      external: ["react", "react-dom", "react-router"],
      format: ["cjs"],
      outDir: enableDevWarnings ? "dist/development" : "dist/production",
      dts: true,
      banner: {
        js: createBanner(pkg.name, pkg.version),
      },
      define: {
        "import.meta.hot": "undefined",
        REACT_ROUTER_VERSION: JSON.stringify(pkg.version),
        __DEV__: JSON.stringify(enableDevWarnings),
      },
    },
    {
      clean: false,
      entry,
      format: ["esm"],
      outDir: enableDevWarnings ? "dist/development" : "dist/production",
      dts: true,
      banner: {
        js: createBanner(pkg.name, pkg.version),
      },
      define: {
        REACT_ROUTER_VERSION: JSON.stringify(pkg.version),
        __DEV__: JSON.stringify(enableDevWarnings),
      },

Review comment (suggestion):
I only added this for the CJS config because the ESM build wasn't bundling `react-router` - but this can be added to both configs is that's safer

Code after:
import { defineConfig } from "tsup";

// @ts-ignore - out of scope
import { createBanner } from "../../build.utils.js";

import pkg from "./package.json";

const entry = ["index.ts", "dom-export.ts", "lib/types/route-module.ts"];

const config = (enableDevWarnings: boolean) =>
  defineConfig([
    {
      clean: false,
      entry,
      format: ["cjs"],
      // Don't bundle `react-router` in sub-exports (i.e., `react-router/dom`)
      external: ["react-router"],
      outDir: enableDevWarnings ? "dist/development" : "dist/production",
      dts: true,
      banner: {
        js: createBanner(pkg.name, pkg.version),
      },
      define: {
        "import.meta.hot": "undefined",
        REACT_ROUTER_VERSION: JSON.stringify(pkg.version),
        __DEV__: JSON.stringify(enableDevWarnings),
      },
    },
    {
      clean: false,
      entry,
      format: ["esm"],
      // We don't do the external thing for `react-router` here because it
      // doesn't get bundled by default in the ESM build, and when we tried it
      // in https://github.com/remix-run/react-router/pull/13497 it changed up
      // some chunk creation that we didn't want to risk having any side effects
      outDir: enableDevWarnings ? "dist/development" : "dist/production",
      dts: true,
      banner: {
        js: createBanner(pkg.name, pkg.version),
Repository: remix-run/react-router
File: packages/react-router/__tests__/router/revalidate-test.ts (TypeScript)

Code before:
    });
    expect(router.state).toMatchObject({
      location: { pathname: "/" },
      navigation: { state: "loading" },
      revalidation: "loading",
      loaderData: { root: "ROOT" },
    });
    expect(count).toBe(3);
    expect(revalidationValue).toBeNull();
    expect(navigationValue).toBeNull();
    expect(navigationValue2).toBeNull();

    // Complete the navigation, which should resolve the revalidation
    await navigationDfd2.resolve("ROOT**");
    expect(router.state).toMatchObject({
      location: { pathname: "/page2" },
      navigation: { state: "idle" },
      revalidation: "idle",
      loaderData: { root: "ROOT**" },
    });
    expect(revalidationValue).toBe("ROOT**");
    expect(navigationValue).toBe("ROOT**");
    expect(navigationValue2).toBe("ROOT**");

    // no-op
    navigationDfd.reject();
    revalidationDfd.reject();
    expect(router.state).toMatchObject({
      location: { pathname: "/page2" },
      navigation: { state: "idle" },
      revalidation: "idle",
      loaderData: { root: "ROOT**" },
    });
    expect(revalidationValue).toBe("ROOT**");
    expect(navigationValue).toBe("ROOT**");
    expect(navigationValue2).toBe("ROOT**");
  });
});


Review comment (suggestion):
This was actually an invalid test before - the `await convertDataStrategyResultToDataResult` on the undefined result of the aborted request was causing the resolution of the navigation dfd to fall to the back of the queue

Code after:
    });
    expect(router.state).toMatchObject({
      location: { pathname: "/" },
      navigation: { state: "loading" },
      revalidation: "loading",
      loaderData: { root: "ROOT" },
    });
    expect(count).toBe(3);
    expect(revalidationValue).toBeNull();
    expect(navigationValue).toBeNull();
    expect(navigationValue2).toBeNull();

    // Complete the navigation, which should resolve the revalidation
    await navigationDfd2.resolve("ROOT**");
    expect(router.state).toMatchObject({
      location: { pathname: "/page2" },
      navigation: { state: "idle" },
      revalidation: "idle",
      loaderData: { root: "ROOT**" },
    });
    // The interrupted revalidation hooks into the next completed navigation
    // so it reflects the end state value
    expect(revalidationValue).toBe("ROOT**");
    // The interim navigation gets interrupted and ends while the router still
    // reflects the original value
    expect(navigationValue).toBe("ROOT");
    expect(navigationValue2).toBe("ROOT**");

    // no-op
    navigationDfd.reject();
    revalidationDfd.reject();
    expect(router.state).toMatchObject({
      location: { pathname: "/page2" },
      navigation: { state: "idle" },
      revalidation: "idle",
      loaderData: { root: "ROOT**" },
    });
    expect(revalidationValue).toBe("ROOT**");
    expect(navigationValue).toBe("ROOT");
    expect(navigationValue2).toBe("ROOT**");
  });
});
Repository: remix-run/react-router
File: packages/react-router/tsup.config.ts (TypeScript)

Code before:
  },
  {
    clean: false,
    entry: [entry],
    format: ["esm"],
    // Don't bundle `react-router` in sub-exports (i.e., `react-router/dom`)
    external: ["react-router"],
    outDir: enableDevWarnings ? "dist/development" : "dist/production",
    dts: true,
    banner: {
      js: createBanner(pkg.name, pkg.version),
    },
    define: {
      REACT_ROUTER_VERSION: JSON.stringify(pkg.version),
      __DEV__: JSON.stringify(enableDevWarnings),
    },
  },
];

export default defineConfig([
  ...config("index.ts", false),
  ...config("index.ts", true),
  ...config("dom-export.ts", false),
  ...config("dom-export.ts", true),
  ...config("lib/types/internal.ts", false),
  ...config("lib/types/internal.ts", true),
]);


Review comment (suggestion):
Does having separate bundle configs create unique files for each bundle? ie. if two bundles depend on the same source file, does each bundle get its own copy? I remember something like that happening before and unifying the entries into a single config fixed it, but I could be misremembering

Code after:
  },
  {
    clean: false,
    entry: [entry],
    format: ["esm"],
    // Don't bundle `react-router` in sub-exports (i.e., `react-router/dom`)
    external: ["react-router"],
    outDir: enableDevWarnings ? "dist/development" : "dist/production",
    dts: true,
    banner: {
      js: createBanner(pkg.name, pkg.version),
    },
    define: {
      REACT_ROUTER_VERSION: JSON.stringify(pkg.version),
      __DEV__: JSON.stringify(enableDevWarnings),
    },
  },
];

export default defineConfig([
  ...config("index.ts", false),
  ...config("index.ts", true),
  ...config("dom-export.ts", false),
  ...config("dom-export.ts", true),
  ...config("internal-export.ts", false),
  ...config("internal-export.ts", true),
]);
Repository: remix-run/react-router
File: packages/react-router/rsc-export.ts (TypeScript)

Code before:
  IsSessionFunction,
  Session,
  SessionData,
  SessionIdStorageStrategy,
  SessionStorage,
  FlashSessionData,
} from "./lib/server-runtime/sessions";
export {
  createSession,
  createSessionStorage,
  isSession,
} from "./lib/server-runtime/sessions";
export { createCookieSessionStorage } from "./lib/server-runtime/sessions/cookieStorage";
export { createMemorySessionStorage } from "./lib/server-runtime/sessions/memoryStorage";

export type {
  DecodeCallServerFunction,
  DecodeFormActionFunction,
  ServerManifestPayload,
  ServerMatch,
  ServerPayload,
  ServerRenderPayload,
  RenderedRoute as ServerRouteManifest,
  ServerRouteMatch,
  ServerRouteObject,
} from "./lib/rsc/server.rsc";
export { matchRSCServerRequest as unstable_matchRSCServerRequest } from "./lib/rsc/server.rsc";


Review comment (performance):
^^ This batch of type exports should get `unstable_` too I think? 

Code after:
  IsSessionFunction,
  Session,
  SessionData,
  SessionIdStorageStrategy,
  SessionStorage,
  FlashSessionData,
} from "./lib/server-runtime/sessions";
export {
  createSession,
  createSessionStorage,
  isSession,
} from "./lib/server-runtime/sessions";
export { createCookieSessionStorage } from "./lib/server-runtime/sessions/cookieStorage";
export { createMemorySessionStorage } from "./lib/server-runtime/sessions/memoryStorage";

export type {
  DecodeCallServerFunction as unstable_DecodeCallServerFunction,
  DecodeFormActionFunction as unstable_DecodeFormActionFunction,
  ServerManifestPayload as unstable_ServerManifestPayload,
  ServerMatch as unstable_ServerMatch,
  ServerPayload as unstable_ServerPayload,
  ServerRenderPayload as unstable_ServerRenderPayload,
  RenderedRoute as ServerRouteManifest,
  ServerRouteMatch as unstable_ServerRouteMatch,
  ServerRouteObject as unstable_ServerRouteObject,
} from "./lib/rsc/server.rsc";
export { matchRSCServerRequest as unstable_matchRSCServerRequest } from "./lib/rsc/server.rsc";
Repository: remix-run/react-router
File: packages/react-router/lib/router/router.ts (TypeScript)

Code before:
    if (fogOfWar.active && fogOfWar.matches) {
      matches = fogOfWar.matches;
    }

    if (!matches) {
      setFetcherError(
        key,
        routeId,
        getInternalRouterError(404, { pathname: normalizedPath }),
        { flushSync }
      );
      return;
    }

    let { path, submission, error } = normalizeNavigateOptions(
      true,
      normalizedPath,
      opts
    );

    if (error) {
      setFetcherError(key, routeId, error, { flushSync });
      return;
    }

    let match = getTargetMatch(matches, path);
    // Create a new context per fetch
    let scopedContext = new unstable_RouterContextProvider(
      init.unstable_getContext ? await init.unstable_getContext() : undefined
    );
    let preventScrollReset = (opts && opts.preventScrollReset) === true;

    if (submission && isMutationMethod(submission.formMethod)) {
      await handleFetcherAction(
        key,
        routeId,
        path,
        match,
        matches,
        scopedContext,
        fogOfWar.active,
        flushSync,
        preventScrollReset,
        submission
      );
      return;
    }

    // Store off the match so we can call it's shouldRevalidate on subsequent
    // revalidations
    fetchLoadMatches.set(key, { routeId, path });

Review comment (suggestion):
In the fog of war case, this `match` is completely ignored and is reassigned once route discovery is done. In an RSC world, if the the root route doesn't have a path, this line throws an error even though its result would never have been used.

To fix this, the creation of this `match` variable is delayed until after route discovery has completed and we know for a fact we have valid matches. This also means we no longer pass it as an argument to `handleFetcherLoader` and `handleFetcherAction`.

Code after:
    if (fogOfWar.active && fogOfWar.matches) {
      matches = fogOfWar.matches;
    }

    if (!matches) {
      setFetcherError(
        key,
        routeId,
        getInternalRouterError(404, { pathname: normalizedPath }),
        { flushSync }
      );
      return;
    }

    let { path, submission, error } = normalizeNavigateOptions(
      true,
      normalizedPath,
      opts
    );

    if (error) {
      setFetcherError(key, routeId, error, { flushSync });
      return;
    }

    // Create a new context per fetch
    let scopedContext = new unstable_RouterContextProvider(
      init.unstable_getContext ? await init.unstable_getContext() : undefined
    );
    let preventScrollReset = (opts && opts.preventScrollReset) === true;

    if (submission && isMutationMethod(submission.formMethod)) {
      await handleFetcherAction(
        key,
        routeId,
        path,
        matches,
        scopedContext,
        fogOfWar.active,
        flushSync,
        preventScrollReset,
        submission
      );
      return;
    }

    // Store off the match so we can call it's shouldRevalidate on subsequent
    // revalidations
    fetchLoadMatches.set(key, { routeId, path });
    await handleFetcherLoader(
      key,
Repository: remix-run/react-router
File: packages/react-router/lib/router/router.ts (TypeScript)

Code before:
    });

    let abortController = new AbortController();
    let fetchRequest = createClientSideRequest(
      init.history,
      path,
      abortController.signal,
      submission
    );

    if (isFogOfWar) {
      let discoverResult = await discoverRoutes(
        requestMatches,
        path,
        fetchRequest.signal,
        key
      );

      if (discoverResult.type === "aborted") {
        return;
      } else if (discoverResult.type === "error") {
        setFetcherError(key, routeId, discoverResult.error, { flushSync });
        return;
      } else if (!discoverResult.matches) {
        setFetcherError(
          key,
          routeId,
          getInternalRouterError(404, { pathname: path }),
          { flushSync }
        );
        return;
      } else {
        requestMatches = discoverResult.matches;
        match = getTargetMatch(requestMatches, path);

        if (detectAndHandle405Error(match)) {
          return;
        }
      }
    }

    // Call the action for the fetcher
    fetchControllers.set(key, abortController);

    let originatingLoadId = incrementingLoadId;
    let fetchMatches = getTargetedDataStrategyMatches(
      mapRouteProperties,
      manifest,
      fetchRequest,
      requestMatches,
      match,

Review comment (suggestion):
The `detectAndHandle405Error` function has been inlined now that this logic is only needed in a single place.

Code after:
        setFetcherError(key, routeId, discoverResult.error, { flushSync });
        return;
      } else if (!discoverResult.matches) {
        setFetcherError(
          key,
          routeId,
          getInternalRouterError(404, { pathname: path }),
          { flushSync }
        );
        return;
      } else {
        requestMatches = discoverResult.matches;
      }
    }

    let match = getTargetMatch(requestMatches, path);

    if (!match.route.action && !match.route.lazy) {
      let error = getInternalRouterError(405, {
        method: submission.formMethod,
        pathname: path,
        routeId: routeId,
      });
      setFetcherError(key, routeId, error, { flushSync });
      return;
    }

    // Call the action for the fetcher
    fetchControllers.set(key, abortController);

    let originatingLoadId = incrementingLoadId;
    let fetchMatches = getTargetedDataStrategyMatches(
      mapRouteProperties,
      manifest,
      fetchRequest,
      requestMatches,
      match,
      hydrationRouteProperties,
      scopedContext
    );
    let actionResults = await callDataStrategy(
      fetchRequest,
      fetchMatches,
      scopedContext,
      key
    );
    let actionResult = actionResults[match.route.id];

    if (fetchRequest.signal.aborted) {
      // We can delete this so long as we weren't aborted by our own fetcher
      // re-submit which would have put _new_ controller is in fetchControllers
Repository: remix-run/react-router
File: packages/create-react-router/utils.ts (TypeScript)

Code before:
  if (key.name === "left") return "left";

  return false;
}

export function stripDirectoryFromPath(dir: string, filePath: string) {
  // Can't just do a regexp replace here since the windows paths mess it up :/
  let stripped = filePath;
  if (
    (dir.endsWith(path.sep) && filePath.startsWith(dir)) ||
    (!dir.endsWith(path.sep) && filePath.startsWith(dir + path.sep))
  ) {
    stripped = filePath.slice(dir.length);
    if (stripped.startsWith(path.sep)) {
      stripped = stripped.slice(1);
    }
  }
  return stripped;
}

// We do not copy these folders from templates so we can ignore them for comparisons
export const IGNORED_TEMPLATE_DIRECTORIES = [".git", "node_modules"];

export async function getDirectoryFilesRecursive(dir: string) {
  return (await readdir(dir, { recursive: true }))
    .filter((file) => {
      let strippedFile = stripDirectoryFromPath(dir, file);
      let parts = strippedFile.split(path.sep);

      return (
        parts.length > 1 && IGNORED_TEMPLATE_DIRECTORIES.includes(parts[0])
      );
    })
    .map((f) => stripDirectoryFromPath(dir, f));
}


Review comment (bug):
i suspect this was preventing recursing into directories before. not sure if its why CI fails but maybe we should check if we still need that behaviour

also the logic reads as if `file` used to be the full path, but now it will be the relative path. so the call to `stripDirectoryFromPath` will do nothing (since it'd never start with `dir`)

so we probably want something like this:

```ts
filter((file) => {
  const parts = file.split(path.sep);
  return parts.length > 1 && IGNORED_TEMPLATE_DIRECTORIES.includes(parts[0]);
});
```

we'll still traverse into the ignored children but filter them out

Code after:
  if (key.name === "left") return "left";

  return false;
}

export function stripDirectoryFromPath(dir: string, filePath: string) {
  // Can't just do a regexp replace here since the windows paths mess it up :/
  let stripped = filePath;
  if (
    (dir.endsWith(path.sep) && filePath.startsWith(dir)) ||
    (!dir.endsWith(path.sep) && filePath.startsWith(dir + path.sep))
  ) {
    stripped = filePath.slice(dir.length);
    if (stripped.startsWith(path.sep)) {
      stripped = stripped.slice(1);
    }
  }
  return stripped;
}

// We do not copy these folders from templates so we can ignore them for comparisons
export const IGNORED_TEMPLATE_DIRECTORIES = [".git", "node_modules"];

export async function getDirectoryFilesRecursive(dir: string) {
  return (await readdir(dir, { recursive: true })).filter((file) => {
    let parts = file.split(path.sep);

    return (
      parts.length <= 1 || !IGNORED_TEMPLATE_DIRECTORIES.includes(parts[0])
    );
  });
}
Repository: remix-run/react-router
File: packages/react-router/lib/server-runtime/crypto.ts (TypeScript)

Code before:
  let hash = btoa(String.fromCharCode(...new Uint8Array(signature))).replace(
    /=+$/,
    ""
  );

  return value + "." + hash;
};

export const unsign = async (
  cookie: string,
  secret: string
): Promise<string | false> => {
  let index = cookie.lastIndexOf(".");
  let value = cookie.slice(0, index);
  let hash = cookie.slice(index + 1);

  let data = encoder.encode(value);

  let key = await createKey(secret, ["verify"]);
  try {
    let signature = byteStringToUint8Array(atob(hash));
    let valid = await crypto.subtle.verify("HMAC", key, signature, data);

    return valid ? value : false;
  } catch (error: unknown) {
    // atob will throw a DOMException with name === 'InvalidCharacterValue'
    // if the signature contains a non-base64 character, which should just
    // be treated as an invalid signature.
    return false;
  }
};

const createKey = async (
  secret: string,
  usages: CryptoKey["usages"]
): Promise<CryptoKey> =>
  crypto.subtle.importKey(
    "raw",
    encoder.encode(secret),
    { name: "HMAC", hash: "SHA-256" },
    false,
    usages
  );

function byteStringToUint8Array(byteString: string): Uint8Array {
  let array = new Uint8Array(byteString.length);

  for (let i = 0; i < byteString.length; i++) {
    array[i] = byteString.charCodeAt(i);
  }


Review comment (suggestion):
```suggestion
    // atob will throw a DOMException with name === 'InvalidCharacterError'
```

Code after:
  let hash = btoa(String.fromCharCode(...new Uint8Array(signature))).replace(
    /=+$/,
    ""
  );

  return value + "." + hash;
};

export const unsign = async (
  cookie: string,
  secret: string
): Promise<string | false> => {
  let index = cookie.lastIndexOf(".");
  let value = cookie.slice(0, index);
  let hash = cookie.slice(index + 1);

  let data = encoder.encode(value);

  let key = await createKey(secret, ["verify"]);
  try {
    let signature = byteStringToUint8Array(atob(hash));
    let valid = await crypto.subtle.verify("HMAC", key, signature, data);

    return valid ? value : false;
  } catch (error: unknown) {
    // atob will throw a DOMException with name === 'InvalidCharacterError'
    // if the signature contains a non-base64 character, which should just
    // be treated as an invalid signature.
    return false;
  }
};

const createKey = async (
  secret: string,
  usages: CryptoKey["usages"]
): Promise<CryptoKey> =>
  crypto.subtle.importKey(
    "raw",
    encoder.encode(secret),
    { name: "HMAC", hash: "SHA-256" },
    false,
    usages
  );

function byteStringToUint8Array(byteString: string): Uint8Array {
  let array = new Uint8Array(byteString.length);

  for (let i = 0; i < byteString.length; i++) {
    array[i] = byteString.charCodeAt(i);
  }
Repository: remix-run/react-router
File: packages/react-router/lib/dom/ssr/routeModules.ts (TypeScript)

Code before:
 * Component, HydrateFallback, and ErrorBoundary
 */
export type LayoutComponent = ComponentType<{
  children: ReactElement<
    unknown,
    ErrorBoundaryComponent | HydrateFallbackComponent | RouteComponent
  >;
}>;

/**
 * A function that defines `<link>` tags to be inserted into the `<head>` of
 * the document on route transitions.
 *
 * @see https://remix.run/route/meta
 */
export interface LinksFunction {
  (): LinkDescriptor[];
}

export interface MetaMatch<
  RouteId extends string = string,
  Loader extends LoaderFunction | ClientLoaderFunction | unknown = unknown,
> {
  id: RouteId;
  pathname: DataRouteMatch["pathname"];
  /** @deprecated Use `loaderData` instead */
  data: Loader extends LoaderFunction | ClientLoaderFunction
    ? SerializeFrom<Loader>
    : unknown;
  loaderData: Loader extends LoaderFunction | ClientLoaderFunction
    ? SerializeFrom<Loader>
    : unknown;
  handle?: RouteHandle;
  params: DataRouteMatch["params"];
  meta: MetaDescriptor[];
  error?: unknown;
}

export type MetaMatches<
  MatchLoaders extends Record<
    string,
    LoaderFunction | ClientLoaderFunction | unknown
  > = Record<string, unknown>,
> = Array<
  {
    [K in keyof MatchLoaders]: MetaMatch<
      Exclude<K, number | symbol>,
      MatchLoaders[K]
    >;
  }[keyof MatchLoaders]
>;

Review comment (suggestion):
```suggestion
  /** @deprecated Use `MetaMatch.loaderData` instead */
```

Code after:
 * Component, HydrateFallback, and ErrorBoundary
 */
export type LayoutComponent = ComponentType<{
  children: ReactElement<
    unknown,
    ErrorBoundaryComponent | HydrateFallbackComponent | RouteComponent
  >;
}>;

/**
 * A function that defines `<link>` tags to be inserted into the `<head>` of
 * the document on route transitions.
 *
 * @see https://remix.run/route/meta
 */
export interface LinksFunction {
  (): LinkDescriptor[];
}

export interface MetaMatch<
  RouteId extends string = string,
  Loader extends LoaderFunction | ClientLoaderFunction | unknown = unknown,
> {
  id: RouteId;
  pathname: DataRouteMatch["pathname"];
  /** @deprecated Use `MetaMatch.loaderData` instead */
  data: Loader extends LoaderFunction | ClientLoaderFunction
    ? SerializeFrom<Loader>
    : unknown;
  loaderData: Loader extends LoaderFunction | ClientLoaderFunction
    ? SerializeFrom<Loader>
    : unknown;
  handle?: RouteHandle;
  params: DataRouteMatch["params"];
  meta: MetaDescriptor[];
  error?: unknown;
}

export type MetaMatches<
  MatchLoaders extends Record<
    string,
    LoaderFunction | ClientLoaderFunction | unknown
  > = Record<string, unknown>,
> = Array<
  {
    [K in keyof MatchLoaders]: MetaMatch<
      Exclude<K, number | symbol>,
      MatchLoaders[K]
    >;
  }[keyof MatchLoaders]
>;