ronantakizawa/codereview-bench huggingface dataset:
Repository: remix-run/react-router
File: packages/react-router-dom/__tests__/link-href-test.tsx (TypeScript)

Code before:
        "#/path?search=value#hash"
      );
    });
  });

  test("fails gracefully on invalid `to` values", () => {
    let warnSpy = jest.spyOn(console, "warn").mockImplementation(() => {});
    let renderer: TestRenderer.ReactTestRenderer;
    TestRenderer.act(() => {
      renderer = TestRenderer.create(
        <MemoryRouter>
          <Routes>
            <Route path="/" element={<Link to="//" />} />
          </Routes>
        </MemoryRouter>
      );
    });

    expect(renderer.root.findByType("a").props.href).toEqual("//");
    expect(warnSpy).toHaveBeenCalledWith(
      '<Link to="//"> contains an invalid URL which will probably break when clicked - please update to a valid URL path.'
    );
    warnSpy.mockRestore();
  });

  test("renders fine when used outside a router context", () => {
    let renderer: TestRenderer.ReactTestRenderer;
    TestRenderer.act(() => {
      renderer = TestRenderer.create(
        <MemoryRouter>
          <Link to="route">Route</Link>
          <Link to="path" relative="path">
            Path
          </Link>
        </MemoryRouter>
      );
    });

    let anchors = renderer.root.findAllByType("a");
    expect(anchors.map((a) => ({ href: a.props.href, text: a.children })))
      .toMatchInlineSnapshot(`
      [
        {
          "href": "/route",
          "text": [
            "Route",
          ],
        },
        {
          "href": "/path",
          "text": [

Review comment (suggestion):
```suggestion
  test("renders fine when used outside a route context", () => {
```

Code after:
        "#/path?search=value#hash"
      );
    });
  });

  test("fails gracefully on invalid `to` values", () => {
    let warnSpy = jest.spyOn(console, "warn").mockImplementation(() => {});
    let renderer: TestRenderer.ReactTestRenderer;
    TestRenderer.act(() => {
      renderer = TestRenderer.create(
        <MemoryRouter>
          <Routes>
            <Route path="/" element={<Link to="//" />} />
          </Routes>
        </MemoryRouter>
      );
    });

    expect(renderer.root.findByType("a").props.href).toEqual("//");
    expect(warnSpy).toHaveBeenCalledWith(
      '<Link to="//"> contains an invalid URL which will probably break when clicked - please update to a valid URL path.'
    );
    warnSpy.mockRestore();
  });

  test("renders fine when used outside a route context", () => {
    let renderer: TestRenderer.ReactTestRenderer;
    TestRenderer.act(() => {
      renderer = TestRenderer.create(
        <MemoryRouter>
          <Link to="route">Route</Link>
          <Link to="path" relative="path">
            Path
          </Link>
        </MemoryRouter>
      );
    });

    let anchors = renderer.root.findAllByType("a");
    expect(anchors.map((a) => ({ href: a.props.href, text: a.children })))
      .toMatchInlineSnapshot(`
      [
        {
          "href": "/route",
          "text": [
            "Route",
          ],
        },
        {
          "href": "/path",
          "text": [
Repository: remix-run/react-router
File: rollup.utils.js (JavaScript)

Code before:
 */`;
}

// Babel plugin to replace `const REACT_ROUTER_VERSION = "0.0.0";` with the
// current version at build time, so we can set it on `window.__reactRouterVersion`
// for consumption by the Core Web Vitals Technology Report
function babelPluginReplaceVersionPlaceholder() {
  return function (babel) {
    var t = babel.types;

    const KIND = "const";
    const NAME = "REACT_ROUTER_VERSION";
    const PLACEHOLDER = "0";

    return {
      visitor: {
        VariableDeclaration: {
          enter: function (path) {
            // Only operate on top-level variables
            if (!path.parentPath.isProgram()) {
              return;
            }

            let { kind, declarations } = path.node;
            if (
              kind === KIND &&
              declarations.length === 1 &&
              declarations[0].id.name === NAME &&
              declarations[0].init?.value === PLACEHOLDER
            ) {
              path.replaceWith(
                t.variableDeclaration(KIND, [
                  t.variableDeclarator(
                    t.identifier(NAME),
                    t.stringLiteral(majorVersion)
                  ),
                ])
              );
            }
          },
        },
      },
    };
  };
}

// Post-build plugin to validate that the version placeholder was replaced
function validateReplacedVersion() {
  return {
    name: "validate-replaced-version",
    writeBundle(_, bundle) {

Review comment (suggestion):
This would get us into a loop since we'd be replacing the placeholder value of `0` with a new value of `0`

Code after:
 */`;
}

// Babel plugin to replace `const REACT_ROUTER_VERSION = "0.0.0";` with the
// current version at build time, so we can set it on `window.__reactRouterVersion`
// for consumption by the Core Web Vitals Technology Report
function babelPluginReplaceVersionPlaceholder() {
  return function (babel) {
    var t = babel.types;

    const KIND = "const";
    const NAME = "REACT_ROUTER_VERSION";
    const PLACEHOLDER = "0";

    return {
      visitor: {
        VariableDeclaration: {
          enter: function (path) {
            // Only operate on top-level variables
            if (!path.parentPath.isProgram()) {
              return;
            }

            // Skip for experimental releases
            if (version.startsWith("0.0.0")) {
              return;
            }

            let { kind, declarations } = path.node;
            if (
              kind === KIND &&
              declarations.length === 1 &&
              declarations[0].id.name === NAME &&
              declarations[0].init?.value === PLACEHOLDER
            ) {
              path.replaceWith(
                t.variableDeclaration(KIND, [
                  t.variableDeclarator(
                    t.identifier(NAME),
                    t.stringLiteral(majorVersion)
                  ),
                ])
              );
            }
          },
        },
      },
    };
  };
}
Repository: remix-run/react-router
File: packages/react-router-dom/dom.ts (TypeScript)

Code before:
  | HTMLInputElement
  | FormData
  | URLSearchParams
  | JsonValue
  | null;

// One-time check for submitter support
let _formDataSupportsSubmitter: boolean | null = null;

function isFormDataSubmitterSupported() {
  if (_formDataSupportsSubmitter === null) {
    try {
      new FormData(
        document.createElement("form"),
        // @ts-expect-error if FormData supports the submitter parameter, this will throw
        0
      );
      _formDataSupportsSubmitter = false;
    } catch (e) {
      _formDataSupportsSubmitter = true;
    }
  }
  return _formDataSupportsSubmitter;
}

export interface FetcherSubmitOptions {
  /**
   * The HTTP method used to submit the form. Overrides `<form method>`.
   * Defaults to "GET".
   */
  method?: HTMLFormMethod;

  /**
   * The action URL path used to submit the form. Overrides `<form action>`.
   * Defaults to the path of the current route.
   */
  action?: string;

  /**
   * The encoding used to submit the form. Overrides `<form encType>`.
   * Defaults to "application/x-www-form-urlencoded".
   */
  encType?: FormEncType;

  /**
   * Determines whether the form action is relative to the route hierarchy or
   * the pathname.  Use this if you want to opt out of navigating the route
   * hierarchy and want to instead route based on /-delimited URL segments
   */
  relative?: RelativeRoutingType;


Review comment (suggestion):
Instead of directly definig these here, have a base private `SharedSubmitOptions` that can be extended.  That way it's not easy to add something fetcher-only and accidentally leak it to non-fetcher cases.  Do this for `FetcherFormProps` as well

Code after:
  | HTMLInputElement
  | FormData
  | URLSearchParams
  | JsonValue
  | null;

// One-time check for submitter support
let _formDataSupportsSubmitter: boolean | null = null;

function isFormDataSubmitterSupported() {
  if (_formDataSupportsSubmitter === null) {
    try {
      new FormData(
        document.createElement("form"),
        // @ts-expect-error if FormData supports the submitter parameter, this will throw
        0
      );
      _formDataSupportsSubmitter = false;
    } catch (e) {
      _formDataSupportsSubmitter = true;
    }
  }
  return _formDataSupportsSubmitter;
}

/**
 * Submit options shared by both navigations and fetchers
 */
interface SharedSubmitOptions {
  /**
   * The HTTP method used to submit the form. Overrides `<form method>`.
   * Defaults to "GET".
   */
  method?: HTMLFormMethod;

  /**
   * The action URL path used to submit the form. Overrides `<form action>`.
   * Defaults to the path of the current route.
   */
  action?: string;

  /**
   * The encoding used to submit the form. Overrides `<form encType>`.
   * Defaults to "application/x-www-form-urlencoded".
   */
  encType?: FormEncType;

  /**
   * Determines whether the form action is relative to the route hierarchy or
   * the pathname.  Use this if you want to opt out of navigating the route
   * hierarchy and want to instead route based on /-delimited URL segments
Repository: remix-run/react-router
File: packages/react-router/lib/router/define-route.ts (TypeScript)

Code before:
  status: number | undefined;
  headers: Headers;
};

// loader
type LoaderArgs<Param extends string> = {
  context: Context;
  request: Request;
  params: Params<Param>;
  response: ResponseStub;
};

// action
type ActionArgs<Param extends string> = {
  context: Context;
  request: Request;
  params: Params<Param>;
  response: ResponseStub;
};

// prettier-ignore
type Params<Param extends string> = Pretty<
  & {[key: string]: string | undefined}
  & {[K in Param as K extends `${string}?` ? never : K]: string}
  & {[K in Param as K extends `${infer P}?` ? P : never]?: string}
>

type IsDefined<T> = undefined extends T ? false : true;

// prettier-ignore
type _LoaderData<
  ServerLoaderData,
  ClientLoaderData,
  ClientLoaderHydrate extends boolean,
  HydrateFallback,
> = Awaited<
  [IsDefined<HydrateFallback>, ClientLoaderHydrate]  extends [true, true] ?
    IsDefined<ClientLoaderData> extends true ? ClientLoaderData :
    undefined
  :
  [IsDefined<ClientLoaderData>, IsDefined<ServerLoaderData>] extends [true, true] ? ServerLoaderData | ClientLoaderData :
  IsDefined<ClientLoaderData> extends true ?
    IsDefined<ClientLoaderHydrate> extends true ? ClientLoaderData :
    ClientLoaderData | undefined
  :
  IsDefined<ServerLoaderData> extends true ? ServerLoaderData :
  undefined
>

type LoaderData<
  ServerLoaderData,

Review comment (suggestion):
Before we checked if `undefined` extended `T`, which is true for unions containing `undefined`. Now we check if `T` is _exactly_ `undefined`. This works since we now default all of the data types in `defineRoute` and `defineRootRoute` generics to be `undefined` (`Data | undefined = undefined`)

Code after:
};

// loader
type LoaderArgs<Param extends string> = {
  context: Context;
  request: Request;
  params: Params<Param>;
  response: ResponseStub;
};

// action
type ActionArgs<Param extends string> = {
  context: Context;
  request: Request;
  params: Params<Param>;
  response: ResponseStub;
};

// prettier-ignore
type Params<Param extends string> = Pretty<
  & {[key: string]: string | undefined}
  & {[K in Param as K extends `${string}?` ? never : K]: string}
  & {[K in Param as K extends `${infer P}?` ? P : never]?: string}
>

type IsDefined<T> = Equal<T, undefined> extends true ? false : true;

// prettier-ignore
type _LoaderData<
  ServerLoaderData,
  ClientLoaderData,
  ClientLoaderHydrate extends boolean,
  HydrateFallback,
> = Awaited<
  [undefined extends HydrateFallback ? false : true, ClientLoaderHydrate]  extends [true, true] ?
    IsDefined<ClientLoaderData> extends true ? ClientLoaderData :
    undefined
  :
  [IsDefined<ClientLoaderData>, IsDefined<ServerLoaderData>] extends [true, true] ? ServerLoaderData | ClientLoaderData :
  IsDefined<ClientLoaderData> extends true ?
    ClientLoaderHydrate extends true ? ClientLoaderData :
    ClientLoaderData | undefined
  :
  IsDefined<ServerLoaderData> extends true ? ServerLoaderData :
  undefined
>

type LoaderData<
  ServerLoaderData,
  ClientLoaderData,
  ClientLoaderHydrate extends boolean,
Repository: remix-run/react-router
File: packages/react-router/lib/dom/ssr/links.ts (TypeScript)

Code before:

  return newMatches;
}

export function getModuleLinkHrefs(
  matches: AgnosticDataRouteMatch[],
  manifestPatch: AssetsManifest
): string[] {
  return dedupeHrefs(
    matches
      .map((match) => {
        let route = manifestPatch.routes[match.route.id];
        let hrefs = [route.module];
        if (route.imports) {
          hrefs = hrefs.concat(route.imports);
        }
        return hrefs;
      })
      .flat(1)
  );
}

// The `<Script>` will render rel=modulepreload for the current page, we don't
// need to include them in a page prefetch, this gives us the list to remove
// while deduping.
function getCurrentPageModulePreloadHrefs(
  matches: AgnosticDataRouteMatch[],
  manifest: AssetsManifest
): string[] {
  return dedupeHrefs(
    matches
      .map((match) => {
        let route = manifest.routes[match.route.id];
        let hrefs = [route.module];

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

        return hrefs;
      })
      .flat(1)
  );
}

function dedupeHrefs(hrefs: string[]): string[] {
  return [...new Set(hrefs)];
}

function sortKeys<Obj extends { [Key in keyof Obj]: Obj[Key] }>(obj: Obj): Obj {
  let sorted = {} as Obj;

Review comment (suggestion):
Apart from the manifest arg having a different name, this function was identical to `getModuleLinkHrefs` above. I went with `manifest` rather than `manifestPatch` as the canonical arg name.

Code after:

  return newMatches;
}

export function getModuleLinkHrefs(
  matches: AgnosticDataRouteMatch[],
  manifest: AssetsManifest
): string[] {
  return dedupeHrefs(
    matches
      .map((match) => {
        let route = manifest.routes[match.route.id];
        let hrefs = [route.module];
        if (route.clientActionModule) {
          hrefs = hrefs.concat(route.clientActionModule);
        }
        if (route.clientLoaderModule) {
          hrefs = hrefs.concat(route.clientLoaderModule);
        }
        if (route.imports) {
          hrefs = hrefs.concat(route.imports);
        }
        return hrefs;
      })
      .flat(1)
  );
}

function dedupeHrefs(hrefs: string[]): string[] {
  return [...new Set(hrefs)];
}

function sortKeys<Obj extends { [Key in keyof Obj]: Obj[Key] }>(obj: Obj): Obj {
  let sorted = {} as Obj;
  let keys = Object.keys(obj).sort();

  for (let key of keys) {
    sorted[key as keyof Obj] = obj[key as keyof Obj];
  }

  return sorted;
}

type KeyedLinkDescriptor<Descriptor extends LinkDescriptor = LinkDescriptor> = {
  key: string;
  link: Descriptor;
};

function dedupeLinkDescriptors<Descriptor extends LinkDescriptor>(
  descriptors: Descriptor[],
  preloads?: string[]
Repository: remix-run/react-router
File: packages/react-router-dev/config/routes.ts (TypeScript)

Code before:

  /**
   * Should be `true` if it is an index route. This disallows child routes.
   */
  index?: boolean;

  /**
   * Should be `true` if the `path` is case-sensitive. Defaults to `false`.
   */
  caseSensitive?: boolean;

  /**
   * The path to the entry point for this route, relative to
   * `config.appDirectory`.
   */
  file: string;

  /**
   * The child routes.
   */
  children?: RouteConfigEntry[];
}

type CreateRoutePath = string | null | undefined;

type RequireAtLeastOne<T> = {
  [K in keyof T]-?: Required<Pick<T, K>> &
    Partial<Pick<T, Exclude<keyof T, K>>>;
}[keyof T];

const createConfigRouteOptionKeys = [
  "id",
  "index",
  "caseSensitive",
] as const satisfies Array<keyof RouteConfigEntry>;
type CreateRouteOptions = Pick<
  RouteConfigEntry,
  (typeof createConfigRouteOptionKeys)[number]
>;
function createRoute(
  path: CreateRoutePath,
  file: string,
  children?: RouteConfigEntry[]
): RouteConfigEntry;
function createRoute(
  path: CreateRoutePath,
  file: string,
  options: RequireAtLeastOne<CreateRouteOptions>,
  children?: RouteConfigEntry[]
): RouteConfigEntry;
function createRoute(

Review comment (refactor):
I thought that this was causing type issues when trying to wrap the route helpers. This turned out not to be the case, but in either case it prompted me to revisit this and I felt like this level of type checking was overkill.

Code after:

  /**
   * Should be `true` if it is an index route. This disallows child routes.
   */
  index?: boolean;

  /**
   * Should be `true` if the `path` is case-sensitive. Defaults to `false`.
   */
  caseSensitive?: boolean;

  /**
   * The path to the entry point for this route, relative to
   * `config.appDirectory`.
   */
  file: string;

  /**
   * The child routes.
   */
  children?: RouteConfigEntry[];
}

type CreateRoutePath = string | null | undefined;

const createConfigRouteOptionKeys = [
  "id",
  "index",
  "caseSensitive",
] as const satisfies Array<keyof RouteConfigEntry>;
type CreateRouteOptions = Pick<
  RouteConfigEntry,
  (typeof createConfigRouteOptionKeys)[number]
>;
function createRoute(
  path: CreateRoutePath,
  file: string,
  children?: RouteConfigEntry[]
): RouteConfigEntry;
function createRoute(
  path: CreateRoutePath,
  file: string,
  options: CreateRouteOptions,
  children?: RouteConfigEntry[]
): RouteConfigEntry;
function createRoute(
  path: CreateRoutePath,
  file: string,
  optionsOrChildren: CreateRouteOptions | RouteConfigEntry[] | undefined,
  children?: RouteConfigEntry[]
): RouteConfigEntry {
Repository: remix-run/react-router
File: packages/react-router/lib/router/utils.ts (TypeScript)

Code before:
/**
 * @private
 */
export const normalizePathname = (pathname: string): string =>
  pathname.replace(/\/+$/, "").replace(/^\/*/, "/");

/**
 * @private
 */
export const normalizeSearch = (search: string): string =>
  !search || search === "?"
    ? ""
    : search.startsWith("?")
    ? search
    : "?" + search;

/**
 * @private
 */
export const normalizeHash = (hash: string): string =>
  !hash || hash === "#" ? "" : hash.startsWith("#") ? hash : "#" + hash;

export type JsonFunction = <Data>(
  data: Data,
  init?: number | ResponseInit
) => Response;

/**
 * This is a shortcut for creating `application/json` responses. Converts `data`
 * to JSON and sets the `Content-Type` header.
 *
 * @category Utils
 */
export const json: JsonFunction = (data, init = {}) => {
  let responseInit = typeof init === "number" ? { status: init } : init;

  let headers = new Headers(responseInit.headers);
  if (!headers.has("Content-Type")) {
    headers.set("Content-Type", "application/json; charset=utf-8");
  }

  return new Response(JSON.stringify(data), {
    ...responseInit,
    headers,
  });
};

export class DataWithResponseInit<D> {
  type: string = "DataWithResponseInit";
  data: D;
  init: ResponseInit | null;

Review comment (refactor):
We had duplicated types for `JsonFunction` (here and from `../server-runtime/responses`) and this one didn't use `TypedResponse`, so replacing it with the version that does track types

Code after:

/**
 * @private
 */
export const normalizePathname = (pathname: string): string =>
  pathname.replace(/\/+$/, "").replace(/^\/*/, "/");

/**
 * @private
 */
export const normalizeSearch = (search: string): string =>
  !search || search === "?"
    ? ""
    : search.startsWith("?")
    ? search
    : "?" + search;

/**
 * @private
 */
export const normalizeHash = (hash: string): string =>
  !hash || hash === "#" ? "" : hash.startsWith("#") ? hash : "#" + hash;

/**
 * This is a shortcut for creating `application/json` responses. Converts `data`
 * to JSON and sets the `Content-Type` header.
 *
 * @category Utils
 */
export const json: JsonFunction = (data, init = {}) => {
  let responseInit = typeof init === "number" ? { status: init } : init;

  let headers = new Headers(responseInit.headers);
  if (!headers.has("Content-Type")) {
    headers.set("Content-Type", "application/json; charset=utf-8");
  }

  return new Response(JSON.stringify(data), {
    ...responseInit,
    headers,
  });
};

export class DataWithResponseInit<D> {
  type: string = "DataWithResponseInit";
  data: D;
  init: ResponseInit | null;

  constructor(data: D, init?: ResponseInit) {
    this.data = data;
    this.init = init || null;
Repository: remix-run/react-router
File: packages/react-router-dev/cli/commands.ts (TypeScript)

Code before:
  appDirectory: string,
  inputFile: string
) {
  await checkForEntry(rootDirectory, appDirectory, serverEntries);
  let contents = await fse.readFile(inputFile, "utf-8");
  return contents;
}

async function createClientEntry(
  rootDirectory: string,
  appDirectory: string,
  inputFile: string
) {
  await checkForEntry(rootDirectory, appDirectory, clientEntries);
  let contents = await fse.readFile(inputFile, "utf-8");
  return contents;
}

export async function typegen(
  root: string,
  flags: { watch: boolean; config?: string }
) {
  root ??= process.cwd();

  if (flags.watch) {
    await Typegen.watch(root, flags.config);
    await new Promise(() => {}); // keep alive
    return;
  }
  await Typegen.run(root, flags.config);
}


Review comment (bug):
Typechecking was failing here due to unwanted 2nd arg, so I also ripped out the obsolete `config` flag as part of this.

Code after:
  appDirectory: string,
  inputFile: string
) {
  await checkForEntry(rootDirectory, appDirectory, serverEntries);
  let contents = await fse.readFile(inputFile, "utf-8");
  return contents;
}

async function createClientEntry(
  rootDirectory: string,
  appDirectory: string,
  inputFile: string
) {
  await checkForEntry(rootDirectory, appDirectory, clientEntries);
  let contents = await fse.readFile(inputFile, "utf-8");
  return contents;
}

export async function typegen(root: string, flags: { watch: boolean }) {
  root ??= process.cwd();

  if (flags.watch) {
    await Typegen.watch(root);
    await new Promise(() => {}); // keep alive
    return;
  }
  await Typegen.run(root);
}