ronantakizawa/codereview-bench huggingface dataset:
Repository: webpack/webpack
File: lib/library/ModuleLibraryPlugin.js (JavaScript)

Code before:
	 * @param {ModuleLibraryPluginOptions} options the plugin options
	 */
	constructor(options) {
		super({
			pluginName: "ModuleLibraryPlugin",
			type: options.type
		});
	}

	/**
	 * Apply the plugin
	 * @param {Compiler} compiler the compiler instance
	 * @returns {void}
	 */
	apply(compiler) {
		super.apply(compiler);

		compiler.hooks.thisCompilation.tap(PLUGIN_NAME, (compilation) => {
			const { onDemandExportsGeneration } =
				ConcatenatedModule.getCompilationHooks(compilation);
			onDemandExportsGeneration.tap(PLUGIN_NAME, (_module) => true);
		});
	}

	/**
	 * @param {Module} module the exporting entry module
	 * @param {string} entryName the name of the entrypoint
	 * @param {LibraryContext<T>} libraryContext context
	 * @returns {void}
	 */
	finishEntryModule(
		module,
		entryName,
		{ options, compilation, compilation: { moduleGraph } }
	) {
		const runtime = getEntryRuntime(compilation, entryName);
		if (options.export) {
			const exportsInfo = moduleGraph.getExportInfo(
				module,
				Array.isArray(options.export) ? options.export[0] : options.export
			);
			exportsInfo.setUsed(UsageState.Used, runtime);
			exportsInfo.canMangleUse = false;
		} else {
			const exportsInfo = moduleGraph.getExportsInfo(module);
			// If the entry module is commonjs, its exports cannot be mangled
			if (module.buildMeta && module.buildMeta.treatAsCommonJs) {
				exportsInfo.setUsedInUnknownWay(runtime);
			} else {
				exportsInfo.setAllKnownExportsUsed(runtime);
			}

Review comment (suggestion):
Sometimes I think we should improve buildMeta and buildInfo types, I think we have it here always, anyway let's do it separately

Code after:
	/**
	 * @param {ModuleLibraryPluginOptions} options the plugin options
	 */
	constructor(options) {
		super({
			pluginName: "ModuleLibraryPlugin",
			type: options.type
		});
	}

	/**
	 * Apply the plugin
	 * @param {Compiler} compiler the compiler instance
	 * @returns {void}
	 */
	apply(compiler) {
		super.apply(compiler);

		compiler.hooks.thisCompilation.tap(PLUGIN_NAME, (compilation) => {
			const { onDemandExportsGeneration } =
				ConcatenatedModule.getCompilationHooks(compilation);
			onDemandExportsGeneration.tap(
				PLUGIN_NAME,
				(module, runtimes, source, finalName) => {
					/** @type {BuildMeta} */
					const buildMeta = module.buildMeta || (module.buildMeta = {});

					const exportsSourceByRuntime =
						buildMeta.exportsSourceByRuntime ||
						(buildMeta.exportsSourceByRuntime = new Map());

					const exportsFinalNameByRuntime =
						buildMeta.exportsFinalNameByRuntime ||
						(buildMeta.exportsFinalNameByRuntime = new Map());

					for (const runtime of runtimes) {
						exportsSourceByRuntime.set(runtime, source);
						exportsFinalNameByRuntime.set(runtime, finalName);
					}

					return true;
				}
			);
		});
	}

	/**
	 * @param {Module} module the exporting entry module
	 * @param {string} entryName the name of the entrypoint
	 * @param {LibraryContext<T>} libraryContext context
	 * @returns {void}
Repository: webpack/webpack
File: test/configCases/node/output-module-external/index.js (JavaScript)

Code before:
import fs from "fs";

it(`should generate import statement for built-in module in node`, () => {
	const path = require("path");
	const content = fs.readFileSync(__filename, "utf-8");

	expect(content).toMatch(/import(.*)from(\s*)"fs"/);
	expect(content).toMatch(/import(.*)from(\s*)"path"/);
	expect(path.basename(__filename)).toBe("bundle0.mjs");
});


Review comment (bug):
Let's add a test case where we import all Node.js modules (just take them from NodeTargetPlugin and generate a module with require's) just to ensure we can replace `require` on `import` for all Node.js modules (It seems like there was a bug with this once, but I could be wrong, let's check it out.)

Code after:
const assertBuiltin = require("assert");
const asyncHooks = require("async_hooks");
const buffer = require("buffer");
const childProcess = require("child_process");
const cluster = require("cluster");
const consoleBuiltin = require("console");
const constants = require("constants");
const crypto = require("crypto");
const dgram = require("dgram");
const dns = require("dns");
const domain = require("domain");
const events = require("events");
const fs = require("fs");
const http = require("http");
const http2 = require("http2");
const https = require("https");
const inspector = require("inspector");
const moduleBuiltin = require("module");
const net = require("net");
const os = require("os");
const path = require("path");
const perfHooks = require("perf_hooks");
const processBuiltin = require("process");
const punycode = require("punycode");
const querystring = require("querystring");
const readline = require("readline");
Repository: webpack/webpack
File: test/configCases/side-effects/asset-module/index.js (JavaScript)

Code before:
import bytes from "./unused.png?bytes";
import inlined from "./unused.png?inline";
import source from "./unused.png?source";
import resource from "./used.png";

it("should not include unused assets", () => {
	expect(resource).toMatch(/\.png/);
	expect(__STATS__.modules.find((m) => m.name.includes("/used.png"))).toEqual(
		expect.objectContaining({
			orphan: false
		})
	);

	for (const stat of __STATS__.modules.filter((m) =>
		m.name.includes("/unused.png")
	)) {
		expect(stat).toEqual(
			expect.objectContaining({
				orphan: true
			})
		);
	}
});


Review comment (suggestion):
What about to rewrite test to see that unused module was removed, we have `__webpack_modules__` to check them in chunk

Code after:
import asset from "./unused.png?asset";
import bytes from "./unused.png?bytes";
import inlined from "./unused.png?inline";
import source from "./unused.png?source";
import resource from "./used.png";

it("should not include unused assets", () => {
	expect(resource).toMatch(/\.png/);

	expect(__webpack_modules__["./used.png"]).toBeDefined();
	expect(__webpack_modules__["./unused.png?asset"]).not.toBeDefined();
	expect(__webpack_modules__["./unused.png?bytes"]).not.toBeDefined();
	expect(__webpack_modules__["./unused.png?inline"]).not.toBeDefined();
	expect(__webpack_modules__["./unused.png?source"]).not.toBeDefined();
});
Repository: webpack/webpack
File: declarations.d.ts (TypeScript)

Code before:
	}
}

declare module "typescript-iterable" {
	// New iterator interfaces from `lib.es2015.iterable.d.ts` for compatibility with old typescript versions and `dispose`
	interface Disposable {
		[Symbol.dispose](): void;
	}

	export interface IteratorObject<T, TReturn = unknown, TNext = unknown>
		extends Iterator<T, TReturn, TNext>, Disposable {
		[Symbol.iterator](): IteratorObject<T, TReturn, TNext>;
	}

	export interface SetIterator<T> extends IteratorObject<
		T,
		BuiltinIteratorReturn,
		unknown
	> {
		[Symbol.iterator](): SetIterator<T>;
	}
}

declare module "assemblyscript/asc" {
	const asc: any;
	export default asc;
}

// There are no typings for @webassemblyjs/ast
declare module "@webassemblyjs/ast" {
	export class AST extends Node {
		type: "Program";
		body: [Module];
	}
	export interface Visitor {
		ModuleImport?: (p: NodePath<ModuleImport>) => void;
		ModuleExport?: (p: NodePath<ModuleExport>) => void;
		Start?: (p: NodePath<Start>) => void;
		Global?: (p: NodePath<Global>) => void;
	}
	export function traverse(node: Node, visitor: Visitor): void;
	export class NodePath<T> {
		node: T;
		remove(): void;
	}
	export class Node {
		type: string;
	}
	export class Identifier extends Node {
		value: string;
	}

Review comment (suggestion):
ts was complaining about dynamic import added declaration here to remove the error

Code after:
	interface ProcessVersions {
		pnp: "1" | "3";
	}
}

declare module "typescript-iterable" {
	// New iterator interfaces from `lib.es2015.iterable.d.ts` for compatibility with old typescript versions and `dispose`
	interface Disposable {
		[Symbol.dispose](): void;
	}

	export interface IteratorObject<T, TReturn = unknown, TNext = unknown>
		extends Iterator<T, TReturn, TNext>, Disposable {
		[Symbol.iterator](): IteratorObject<T, TReturn, TNext>;
	}

	export interface SetIterator<T> extends IteratorObject<
		T,
		BuiltinIteratorReturn,
		unknown
	> {
		[Symbol.iterator](): SetIterator<T>;
	}
}

declare module "assemblyscript/asc" {
	export * from "types:assemblyscript/cli/index";
	import * as asc from "types:assemblyscript/cli/index";
	export default asc;
}

// There are no typings for @webassemblyjs/ast
declare module "@webassemblyjs/ast" {
	export class AST extends Node {
		type: "Program";
		body: [Module];
	}
	export interface Visitor {
		ModuleImport?: (p: NodePath<ModuleImport>) => void;
		ModuleExport?: (p: NodePath<ModuleExport>) => void;
		Start?: (p: NodePath<Start>) => void;
		Global?: (p: NodePath<Global>) => void;
	}
	export function traverse(node: Node, visitor: Visitor): void;
	export class NodePath<T> {
		node: T;
		remove(): void;
	}
	export class Node {
		type: string;
	}
Repository: webpack/webpack
File: declarations.d.ts (TypeScript)

Code before:
	// New iterator interfaces from `lib.es2015.iterable.d.ts` for compatibility with old typescript versions and `dispose`
	interface Disposable {
		[Symbol.dispose](): void;
	}

	export interface IteratorObject<T, TReturn = unknown, TNext = unknown>
		extends Iterator<T, TReturn, TNext>, Disposable {
		[Symbol.iterator](): IteratorObject<T, TReturn, TNext>;
	}

	export interface SetIterator<T> extends IteratorObject<
		T,
		BuiltinIteratorReturn,
		unknown
	> {
		[Symbol.iterator](): SetIterator<T>;
	}
}

declare module "assemblyscript/asc" {
	const asc: any;
	export default asc;
}

// There are no typings for @webassemblyjs/ast
declare module "@webassemblyjs/ast" {
	export class AST extends Node {
		type: "Program";
		body: [Module];
	}
	export interface Visitor {
		ModuleImport?: (p: NodePath<ModuleImport>) => void;
		ModuleExport?: (p: NodePath<ModuleExport>) => void;
		Start?: (p: NodePath<Start>) => void;
		Global?: (p: NodePath<Global>) => void;
	}
	export function traverse(node: Node, visitor: Visitor): void;
	export class NodePath<T> {
		node: T;
		remove(): void;
	}
	export class Node {
		type: string;
	}
	export class Identifier extends Node {
		value: string;
	}
	export class Start extends Node {
		index: Identifier;
	}
	export class Module extends Node {

Review comment (suggestion):
`assemblyscript/asc` has own types, we need to use it - https://github.com/AssemblyScript/assemblyscript/blob/main/dist/asc.d.ts

Code after:

declare module "typescript-iterable" {
	// New iterator interfaces from `lib.es2015.iterable.d.ts` for compatibility with old typescript versions and `dispose`
	interface Disposable {
		[Symbol.dispose](): void;
	}

	export interface IteratorObject<T, TReturn = unknown, TNext = unknown>
		extends Iterator<T, TReturn, TNext>, Disposable {
		[Symbol.iterator](): IteratorObject<T, TReturn, TNext>;
	}

	export interface SetIterator<T> extends IteratorObject<
		T,
		BuiltinIteratorReturn,
		unknown
	> {
		[Symbol.iterator](): SetIterator<T>;
	}
}

declare module "assemblyscript/asc" {
	export * from "types:assemblyscript/cli/index";
	import * as asc from "types:assemblyscript/cli/index";
	export default asc;
}

// There are no typings for @webassemblyjs/ast
declare module "@webassemblyjs/ast" {
	export class AST extends Node {
		type: "Program";
		body: [Module];
	}
	export interface Visitor {
		ModuleImport?: (p: NodePath<ModuleImport>) => void;
		ModuleExport?: (p: NodePath<ModuleExport>) => void;
		Start?: (p: NodePath<Start>) => void;
		Global?: (p: NodePath<Global>) => void;
	}
	export function traverse(node: Node, visitor: Visitor): void;
	export class NodePath<T> {
		node: T;
		remove(): void;
	}
	export class Node {
		type: string;
	}
	export class Identifier extends Node {
		value: string;
	}
	export class Start extends Node {
Repository: withastro/astro
File: packages/create-astro/test/create-astro.test.js (JavaScript)

Code before:
import fs from 'fs';
import path from 'path';
import { fileURLToPath } from 'url';
import http from 'http';
import { suite } from 'uvu';
import execa from 'execa';
import del from 'del';
import glob from 'tiny-glob';
import * as assert from 'uvu/assert';
import { TEMPLATES } from '../dist/templates.js';

// config
const GITHUB_SHA = process.env.GITHUB_SHA || execa.sync('git', ['rev-parse', 'HEAD']).stdout; // process.env.GITHUB_SHA will be set in CI; if testing locally execa() will gather this
const MAX_TEST_TIME = 60000; // maximum time a test may take (60s)
const TIMER = {}; // keep track of every test’s run time (uvu requires manual setup for this)
const FIXTURES_DIR = path.join(fileURLToPath(path.dirname(import.meta.url)), 'fixtures');

// helper
async function fetch(url) {
  return new Promise((resolve) => {
    http
      .get(url, (res) => {
        let body = '';
        res.on('data', (chunk) => {
          body += chunk;
        });
        res.on('end', () => resolve({ statusCode: res.statusCode, body }));
      })
      .on('error', (err) => {
        reject(err);

Review comment (bug):
`uvu` was removed for this test because it was just swallowing error after error, making it impossible to debug in CI

Code after:
import fs from 'fs';
import path from 'path';
import { fileURLToPath } from 'url';
import http from 'http';
import { green, red } from 'kleur/colors';
import execa from 'execa';
import glob from 'tiny-glob';
import { TEMPLATES } from '../dist/templates.js';

// config
const GITHUB_SHA = process.env.GITHUB_SHA || execa.sync('git', ['rev-parse', 'HEAD']).stdout; // process.env.GITHUB_SHA will be set in CI; if testing locally execa() will gather this
const FIXTURES_DIR = path.join(fileURLToPath(path.dirname(import.meta.url)), 'fixtures');

// helpers
async function fetch(url) {
  return new Promise((resolve, reject) => {
    http
      .get(url, (res) => {
        // not OK
        if (res.statusCode !== 200) {
          reject(res.statusCode);
          return;
        }

        // OK
        let body = '';
        res.on('data', (chunk) => {
          body += chunk;
        });
        res.on('end', () => resolve({ statusCode: res.statusCode, body }));
Repository: withastro/astro
File: packages/astro/src/compiler/transform/doctype.ts (TypeScript)

Code before:
import { Transformer } from '../../@types/transformer';

/** Transform <!doctype> tg */
export default function (_opts: { filename: string; fileID: string }): Transformer {
  let hasDoctype = false;

  return {
    visitors: {
      html: {
        Element: {
          enter(node, parent, _key, index) {
            if (node.name === '!doctype' || node.name === '!DOCTYPE') {
              hasDoctype = true;
            }
            if (node.name === 'html' && !hasDoctype) {
              const dtNode = {
                start: 0,
                end: 0,
                attributes: [{ type: 'Attribute', name: 'html', value: true, start: 0, end: 0 }],
                children: [],
                name: '!doctype',
                type: 'Element',
              };
              (parent.children || []).splice(index, 0, dtNode);
              hasDoctype = true;
            }
          },
        },
      },
    },
    async finalize() {
      // Nothing happening here.
    },
  };
}


Review comment (suggestion):
Just checked the spec. It looks like mixed-case `!doctype` is valid as well, so this should probably check for `node.name.toLowerCase() === '!doctype'`

https://html.spec.whatwg.org/multipage/parsing.html#doctype-name-state

Code after:
import { Transformer } from '../../@types/transformer';

/** Transform <!doctype> tg */
export default function (_opts: { filename: string; fileID: string }): Transformer {
  let hasDoctype = false;

  return {
    visitors: {
      html: {
        Element: {
          enter(node, parent, _key, index) {
            if (node.name.toLowerCase() === '!doctype') {
              hasDoctype = true;
            }
            if (node.name === 'html' && !hasDoctype) {
              const dtNode = {
                start: 0,
                end: 0,
                attributes: [{ type: 'Attribute', name: 'html', value: true, start: 0, end: 0 }],
                children: [],
                name: '!doctype',
                type: 'Element',
              };
              (parent.children || []).splice(index, 0, dtNode);
              hasDoctype = true;
            }
          },
        },
      },
    },
    async finalize() {
      // Nothing happening here.
    },
  };
}
Repository: withastro/astro
File: packages/astro-parser/src/parse/read/expression.ts (TypeScript)

Code before:
  do {
    const char = next_char(state);
    if (char === '\n') {
      break;
    }
  } while (in_bounds(state));
}

const voidElements = new Set(['area', 'base', 'br', 'col', 'command', 'embed', 'hr', 'img', 'input', 'keygen', 'link', 'meta', 'param', 'source', 'track', 'wbr']);

function consume_tag(state: ParseState) {
  const start = state.index - 1;
  let tagName = '';
  let inTag = false;
  let inStart = true;
  let selfClosed = false;
  let inClose = false;

  let bracketIndex = 1;
  do {
    const char = next_char(state);

    switch (char) {
      case "'":
      case '"': {
        consume_string(state, char);
        break;
      }
      case '<': {
        inTag = false;
        tagName = '';

        if (peek_nonwhitespace(state) === '/') {
          inClose = true;
          bracketIndex--;
        } else {
          inStart = true;
          bracketIndex++;
        }
        break;
      }
      case '>': {
        // An arrow function, probably
        if (!inStart && !inClose) {
          break;
        }

        bracketIndex--;

        const addExpectedBrackets =
          // Void elements don't need a closing

Review comment (bug):
This was the bug in the parser: we forgot to look for "`" as a valid string character when parsing HTML tags (hence why it only broke when inlined in tags, and wouldn’t break elsewhere)

This behavior was inconsistent with the rest of the parser which usually looks for all 3 string characters. And as far as I can tell, this deviation in this one spot seems accidental.

Code after:
  do {
    const char = next_char(state);
    if (char === '\n') {
      break;
    }
  } while (in_bounds(state));
}

const voidElements = new Set(['area', 'base', 'br', 'col', 'command', 'embed', 'hr', 'img', 'input', 'keygen', 'link', 'meta', 'param', 'source', 'track', 'wbr']);

function consume_tag(state: ParseState) {
  const start = state.index - 1;
  let tagName = '';
  let inTag = false;
  let inStart = true;
  let selfClosed = false;
  let inClose = false;

  let bracketIndex = 1;
  do {
    const char = next_char(state);

    switch (char) {
      case "'":
      case '"':
      case '`': {
        consume_string(state, char);
        break;
      }
      case '<': {
        inTag = false;
        tagName = '';

        if (peek_nonwhitespace(state) === '/') {
          inClose = true;
          bracketIndex--;
        } else {
          inStart = true;
          bracketIndex++;
        }
        break;
      }
      case '>': {
        // An arrow function, probably
        if (!inStart && !inClose) {
          break;
        }

        bracketIndex--;

        const addExpectedBrackets =
Repository: withastro/astro
File: packages/astro/src/@types/astro.ts (TypeScript)

Code before:
import type { ImportSpecifier, ImportDefaultSpecifier, ImportNamespaceSpecifier } from '@babel/types';

export interface AstroConfigRaw {
  dist: string;
  projectRoot: string;
  srcRoot: string;
  pages: string;
  public: string;
  jsx?: string;
}

export interface AstroMarkdownOptions {
  /** Enable or disable footnotes syntax extension */
  footnotes: boolean;
  /** Enable or disable GitHub-flavored Markdown syntax extension */
  gfm: boolean;
}
export interface AstroConfig {
  dist: string;
  projectRoot: URL;
  pages: URL;
  public: URL;
  srcRoot: URL;
  renderers?: string[];
  /** Options for rendering markdown content */
  markdownOptions?: Partial<AstroMarkdownOptions>;
  /** Options specific to `astro build` */
  buildOptions: {
    /** Your public domain, e.g.: https://my-site.dev/. Used to generate sitemaps and canonical URLs. */
    site?: string;
    /** Generate sitemap (set to "false" to disable) */

Review comment (suggestion):
Probably should call this just `src` to match pages/public which don't have the root. only projectRoot does (maybe should change that later, idk)

Code after:
import type { ImportSpecifier, ImportDefaultSpecifier, ImportNamespaceSpecifier } from '@babel/types';

export interface AstroConfigRaw {
  dist: string;
  projectRoot: string;
  src: string;
  pages: string;
  public: string;
  jsx?: string;
}

export interface AstroMarkdownOptions {
  /** Enable or disable footnotes syntax extension */
  footnotes: boolean;
  /** Enable or disable GitHub-flavored Markdown syntax extension */
  gfm: boolean;
}
export interface AstroConfig {
  dist: string;
  projectRoot: URL;
  pages: URL;
  public: URL;
  src: URL;
  renderers?: string[];
  /** Options for rendering markdown content */
  markdownOptions?: Partial<AstroMarkdownOptions>;
  /** Options specific to `astro build` */
  buildOptions: {
    /** Your public domain, e.g.: https://my-site.dev/. Used to generate sitemaps and canonical URLs. */
    site?: string;
    /** Generate sitemap (set to "false" to disable) */
Repository: withastro/astro
File: packages/astro/src/core/routing/astro-designed-error-pages.ts (TypeScript)

Code before:
import notFoundTemplate from '../../template/4xx.js';
import type { ComponentInstance, ManifestData } from '../../types/astro.js';
import type { RouteData } from '../../types/public/internal.js';
import { DEFAULT_404_COMPONENT, DEFAULT_500_COMPONENT } from '../constants.js';

export const DEFAULT_404_ROUTE: RouteData = {
	component: DEFAULT_404_COMPONENT,
	generate: () => '',
	params: [],
	pattern: /\/404/,
	prerender: false,
	pathname: '/404',
	segments: [[{ content: '404', dynamic: false, spread: false }]],
	type: 'page',
	route: '/404',
	fallbackRoutes: [],
	isIndex: false,
	origin: 'core',
};

export const DEFAULT_500_ROUTE: RouteData = {
	component: DEFAULT_500_COMPONENT,
	generate: () => '',
	params: [],
	pattern: /\/500/,
	prerender: false,
	pathname: '/500',
	segments: [[{ content: '500', dynamic: false, spread: false }]],
	type: 'page',
	route: '/500',
	fallbackRoutes: [],
	isIndex: false,
	origin: 'core',
};

export function ensure404Route(manifest: ManifestData) {
	if (!manifest.routes.some((route) => route.route === '/404')) {
		manifest.routes.push(DEFAULT_404_ROUTE);
	}
	return manifest;
}

async function default404Page({ pathname }: { pathname: string }) {

Review comment (suggestion):
Although we use "core" to refer to the `astro` package, is this as clear to users of the API as it is to us? Maybe `origin: 'astro'` would be better for users.

Code after:
import notFoundTemplate from '../../template/4xx.js';
import type { ComponentInstance, ManifestData } from '../../types/astro.js';
import type { RouteData } from '../../types/public/internal.js';
import { DEFAULT_404_COMPONENT, DEFAULT_500_COMPONENT } from '../constants.js';

export const DEFAULT_404_ROUTE: RouteData = {
	component: DEFAULT_404_COMPONENT,
	generate: () => '',
	params: [],
	pattern: /\/404/,
	prerender: false,
	pathname: '/404',
	segments: [[{ content: '404', dynamic: false, spread: false }]],
	type: 'page',
	route: '/404',
	fallbackRoutes: [],
	isIndex: false,
	origin: 'internal',
};

export const DEFAULT_500_ROUTE: RouteData = {
	component: DEFAULT_500_COMPONENT,
	generate: () => '',
	params: [],
	pattern: /\/500/,
	prerender: false,
	pathname: '/500',
	segments: [[{ content: '500', dynamic: false, spread: false }]],
	type: 'page',
	route: '/500',
	fallbackRoutes: [],
	isIndex: false,
	origin: 'internal',
};

export function ensure404Route(manifest: ManifestData) {
	if (!manifest.routes.some((route) => route.route === '/404')) {
		manifest.routes.push(DEFAULT_404_ROUTE);
	}
	return manifest;
}

async function default404Page({ pathname }: { pathname: string }) {