ronantakizawa/codereview-bench huggingface dataset:
Repository: parcel-bundler/parcel
File: packages/core/test-utils/src/utils.js (JavaScript)

Code before:

    getElementsByTagName() {
      return [head];
    },

    createEvent() {
      // For Vue
      return {timeStamp: Date.now()};
    },

    getElementById(id) {
      if (id !== '__parcel__error__overlay__') return fakeElement;
    },

    body: {
      appendChild() {
        return null;
      },
    },

    currentScript: null,
  };

  var exports = {};

  class customError {
    constructor(message) {
      const customError = new Error(message);
      const stackStart = customError.stack.indexOf('at new customError');
      const stack = customError.stack
        .slice(stackStart, customError.stack.indexOf('at Script.runInContext'))
        .split('\n');
      stack.shift();
      stack.pop();
      for (let [i, line] of stack.entries()) {
        stack[i] = line.replace(
          /( ?.* )\(?(.*)\)?$/,
          (_, prefix, path) =>
            prefix +
            (path.endsWith(')')
              ? `(http://localhost/${path.slice(0, path.length - 1)})`
              : `http://localhost/${path}`),
        );
      }
      customError.stack =
        customError.stack.slice(0, stackStart).replace(/ +$/, '') +
        stack.join('\n');
      return customError;
    }
  }


Review comment (bug):
nit: UpperCamelCase? and maybe a better name

Also, I wonder if this should be a function with a `.prototype` property that's the current Error.prototype. Otherwise assumptions about this being the "real" Error may fail.

Code after:
      assert.deepEqual(actualBundle.assets, bundle.assets);
    }
  }
}

export function normaliseNewlines(text: string): string {
  return text.replace(/(\r\n|\n|\r)/g, '\n');
}

function prepareBrowserContext(
  bundle: PackagedBundle,
  globals: mixed,
): {|
  ctx: vm$Context,
  promises: Array<Promise<mixed>>,
|} {
  // for testing dynamic imports
  const fakeElement = {
    remove() {},
  };

  const head = {
    children: [],
    appendChild(el) {
      head.children.push(el);

      if (el.tag === 'script') {
        let {deferred, promise} = makeDeferredWithPromise();
        promises.push(promise);
        setTimeout(function() {
          let pathname = url.parse(el.src).pathname;
          let file = path.join(bundle.target.distDir, pathname);

          new vm.Script(
            // '"use strict";\n' +
            overlayFS.readFileSync(file, 'utf8'),
            {
              filename: pathname.slice(1),
            },
          ).runInContext(ctx);

          el.onload();
          deferred.resolve();
        }, 0);
      } else if (typeof el.onload === 'function') {
        el.onload();
      }
    },
  };

  let promises = [];
Repository: parcel-bundler/parcel
File: packages/core/core/src/Transformation.js (JavaScript)

Code before:
      if (ast) {
        asset.setAST(ast);
        asset.isASTDirty = false;
      }
    }

    // Transform.
    let results = await normalizeAssets(
      // $FlowFixMe
      await transformer.transform({
        asset: new MutableAsset(asset),
        ast: asset.ast,
        config,
        options: pipeline.pluginOptions,
        resolve,
        logger,
      }),
    );

    // Create generate function that can be called later
    pipeline.generate = (input: UncommittedAsset): Promise<GenerateOutput> => {
      let generate = transformer.generate?.bind(transformer);
      let ast = input.ast;
      if (generate && ast) {
        let generated = generate({
          asset: new Asset(input),
          ast,
          options: pipeline.pluginOptions,
          logger,
        });
        input.clearAST();
        return Promise.resolve(generated);
      }

      throw new Error(
        'Asset has an AST but no generate method is available on the transform',
      );
    };

    return results;
  }
}

type Pipeline = {|
  id: string,
  transformers: Array<TransformerWithNameAndConfig>,
  options: ParcelOptions,
  pluginOptions: PluginOptions,
  resolverRunner: ResolverRunner,
  workerApi: WorkerApi,
  generate?: GenerateFunc,

Review comment (suggestion):
```suggestion
      let ast = input.ast;
      let asset = new Asset(input);
      if (transformer.generate && ast) {
        let generated = transformer.generate({
          asset,
```

Code after:
        logger,
      });
      if (ast) {
        asset.setAST(ast);
        asset.isASTDirty = false;
      }
    }

    // Transform.
    let results = await normalizeAssets(
      // $FlowFixMe
      await transformer.transform({
        asset: new MutableAsset(asset),
        ast: asset.ast,
        config,
        options: pipeline.pluginOptions,
        resolve,
        logger,
      }),
    );

    // Create generate function that can be called later
    pipeline.generate = (input: UncommittedAsset): Promise<GenerateOutput> => {
      let ast = input.ast;
      let asset = new Asset(input);
      if (transformer.generate && ast) {
        let generated = transformer.generate({
          asset,
          ast,
          options: pipeline.pluginOptions,
          logger,
        });
        input.clearAST();
        return Promise.resolve(generated);
      }

      throw new Error(
        'Asset has an AST but no generate method is available on the transform',
      );
    };

    return results;
  }
}

type Pipeline = {|
  id: string,
  transformers: Array<TransformerWithNameAndConfig>,
  options: ParcelOptions,
  pluginOptions: PluginOptions,
  resolverRunner: ResolverRunner,
Repository: parcel-bundler/parcel
File: packages/reporters/dev-server/src/Server.js (JavaScript)

Code before:
  }

  sendIndex(req: Request, res: Response) {
    if (this.bundleGraph) {
      // If the main asset is an HTML file, serve it
      let htmlBundleFilePaths = [];
      this.bundleGraph.traverseBundles(bundle => {
        if (bundle.type === 'html' && bundle.isEntry) {
          htmlBundleFilePaths.push(bundle.filePath);
        }
      });

      htmlBundleFilePaths = htmlBundleFilePaths.map(p => {
        return `/${relativePath(this.options.distDir, p, false)}`;
      });

      let indexFilePath = null;
      if (htmlBundleFilePaths.length === 1) {
        indexFilePath = htmlBundleFilePaths[0];
      } else {
        let reqDirectory = getUrlDirectory(req.url);
        indexFilePath = htmlBundleFilePaths
          .filter(v => {
            let dir = getUrlDirectory(v);
            let filename = v.replace(dir, '');
            let withoutExtension = filename.replace(path.extname(filename), '');
            return (
              withoutExtension === '/index' && reqDirectory.startsWith(dir)
            );
          })
          .sort((a, b) => {
            return b.length - a.length;
          })[0];
      }

      if (indexFilePath) {
        req.url = indexFilePath;
        this.serveBundle(req, res, () => this.send404(req, res));
      } else {
        this.send404(req, res);
      }
    } else {
      this.send404(req, res);
    }
  }

  async serveBundle(
    req: Request,
    res: Response,
    next: NextFunction,
  ): Promise<void> {

Review comment (suggestion):
`v` is always a url to that bundle,
so `withoutExtension` could just be `path.posix.basename(v, path.posix.extname(v))`? 

Which is actually also the same as `v.endsWith("/index.html")`

Code after:
          htmlBundleFilePaths.push(bundle.filePath);
        }
      });

      htmlBundleFilePaths = htmlBundleFilePaths.map(p => {
        return `/${relativePath(this.options.distDir, p, false)}`;
      });

      let indexFilePath = null;
      if (htmlBundleFilePaths.length === 1) {
        indexFilePath = htmlBundleFilePaths[0];
      } else {
        indexFilePath = htmlBundleFilePaths
          .filter(v => {
            let dir = path.posix.dirname(v);
            let withoutExtension = path.posix.basename(
              v,
              path.posix.extname(v),
            );
            return withoutExtension === 'index' && req.url.startsWith(dir);
          })
          .sort((a, b) => {
            return b.length - a.length;
          })[0];
      }

      if (indexFilePath) {
        req.url = indexFilePath;
        this.serveBundle(req, res, () => this.send404(req, res));
      } else {
        this.send404(req, res);
      }
    } else {
      this.send404(req, res);
    }
  }

  async serveBundle(
    req: Request,
    res: Response,
    next: NextFunction,
  ): Promise<void> {
    let bundleGraph = this.bundleGraph;
    if (bundleGraph) {
      let {pathname} = url.parse(req.url);
      if (!pathname) {
        this.send500(req, res);
        return;
      }

      let requestedPath = path.normalize(pathname.slice(1));
Repository: parcel-bundler/parcel
File: packages/transformers/babel/src/config.js (JavaScript)

Code before:
    return;
  }

  // Invalidate when any babel config file is added.
  for (let fileName of BABEL_CONFIG_FILENAMES) {
    config.invalidateOnFileCreate({
      fileName,
      aboveFilePath: config.searchPath,
    });
  }

  // Do nothing if we cannot resolve any babel config filenames. Checking using our own
  // config resolution (which is cached) is much faster than relying on babel.
  if (
    !(await resolveConfig(
      options.inputFS,
      config.searchPath,
      BABEL_CONFIG_FILENAMES,
      options.projectRoot,
    ))
  ) {
    await buildDefaultBabelConfig(options, config);
    return;
  }

  const babelCore: BabelCore = await options.packageManager.require(
    '@babel/core',
    config.searchPath,
    {
      range: '^7.12.0',
      saveDev: true,
      shouldAutoInstall: options.shouldAutoInstall,
    },
  );

  let babelOptions = {
    filename: config.searchPath,
    cwd: options.projectRoot,
    envName:
      options.env.BABEL_ENV ??
      options.env.NODE_ENV ??
      (options.mode === 'production' || options.mode === 'development'
        ? options.mode
        : null) ??
      'development',
    showIgnoredFiles: true,
  };

  let partialConfig: ?{|
    [string]: any,
  |} = await babelCore.loadPartialConfigAsync(babelOptions);

Review comment (performance):
This won't get tracked for cache invalidation. We need to call `config.addDevDependency` here as well.

Code after:
    return;
  }

  // Invalidate when any babel config file is added.
  for (let fileName of BABEL_CONFIG_FILENAMES) {
    config.invalidateOnFileCreate({
      fileName,
      aboveFilePath: config.searchPath,
    });
  }

  // Do nothing if we cannot resolve any babel config filenames. Checking using our own
  // config resolution (which is cached) is much faster than relying on babel.
  if (
    !(await resolveConfig(
      options.inputFS,
      config.searchPath,
      BABEL_CONFIG_FILENAMES,
      options.projectRoot,
    ))
  ) {
    await buildDefaultBabelConfig(options, config);
    return;
  }

  const babelCore: BabelCore = await options.packageManager.require(
    '@babel/core',
    config.searchPath,
    {
      range: '^7.12.0',
      saveDev: true,
      shouldAutoInstall: options.shouldAutoInstall,
    },
  );
  config.addDevDependency({
    moduleSpecifier: '@babel/core',
    resolveFrom: config.searchPath,
  });

  let babelOptions = {
    filename: config.searchPath,
    cwd: options.projectRoot,
    envName:
      options.env.BABEL_ENV ??
      options.env.NODE_ENV ??
      (options.mode === 'production' || options.mode === 'development'
        ? options.mode
        : null) ??
      'development',
    showIgnoredFiles: true,
  };
Repository: parcel-bundler/parcel
File: packages/core/package-manager/src/JSONParseStream.js (JavaScript)

Code before:
// Pair with split2 to parse stream of newline-delimited text.
export default class JSONParseStream extends Transform {
  constructor(options: mixed) {
    super({...options, objectMode: true});
  }

  // $FlowFixMe We are in object mode, so we emit objects, not strings
  _transform(
    chunk: Buffer | string,
    encoding: string,
    callback: (err: ?Error, parsed: ?JSONObject) => mixed,
  ) {
    try {
      let parsed;
      try {
        parsed = JSON.parse(chunk.toString());
      } catch (e) {
        return;
      }
      callback(null, parsed);
    } catch (err) {
      callback(err);
    }
  }
}


Review comment (bug):
Maybe a comment here that we're intentionally not calling the callback with these errors because some streaming JSON implementations leak non-json? (At least that's what I assume is going on here)

Additionally, maybe we could log these at verbose/debug levels of the logger?

Code after:

// Transforms chunks of json strings to parsed objects.
// Pair with split2 to parse stream of newline-delimited text.
export default class JSONParseStream extends Transform {
  constructor(options: mixed) {
    super({...options, objectMode: true});
  }

  // $FlowFixMe We are in object mode, so we emit objects, not strings
  _transform(
    chunk: Buffer | string,
    encoding: string,
    callback: (err: ?Error, parsed: ?JSONObject) => mixed,
  ) {
    try {
      let parsed;
      try {
        parsed = JSON.parse(chunk.toString());
      } catch (e) {
        // Be permissive and ignoreJSON parse errors in case there was
        // a non-JSON line in the package manager's stdout.
        logger.verbose({
          message: 'Ignored invalid JSON message: ' + chunk.toString(),
          origin: '@parcel/package-manager',
        });
        return;
      }
      callback(null, parsed);
    } catch (err) {
      callback(err);
    }
  }
}
Repository: parcel-bundler/parcel
File: packages/utils/node-resolver-core/src/NodeResolver.js (JavaScript)

Code before:
    pkg: InternalPackageJSON | null,
  ): Promise<?ResolvedAlias> {
    let localAliases = await this.resolvePackageAliases(filename, env, pkg);
    if (localAliases) {
      return localAliases;
    }

    // First resolve local package aliases, then project global ones.
    return this.resolvePackageAliases(filename, env, this.rootPackage);
  }

  async resolvePackageAliases(
    filename: string,
    env: Env,
    pkg: InternalPackageJSON | null,
  ): Promise<?ResolvedAlias> {
    if (!pkg) {
      return null;
    }

    let pkgKeys = ['source', 'alias'];
    if (env.isBrowser()) pkgKeys.push('browser');

    for (let pkgKey of pkgKeys) {
      let pkgKeyValue = pkg[pkgKey];
      if (!Array.isArray(pkgKeyValue)) {
        let alias = await this.getAlias(filename, pkg, pkgKeyValue);
        if (alias != null) {
          return alias;
        }
      }
    }
    return null;
  }

  async getAlias(
    filename: FilePath,
    pkg: InternalPackageJSON,
    aliases: ?Aliases,
  ): Promise<?ResolvedAlias> {
    if (!filename || !aliases || typeof aliases !== 'object') {
      return null;
    }

    let dir = pkg.pkgdir;
    let alias;

    // If filename is an absolute path, get one relative to the package.json directory.
    if (path.isAbsolute(filename)) {
      filename = relativePath(dir, filename);
      alias = this.lookupAlias(aliases, filename);

Review comment (performance):
Making this function use static references rather than dynamic property accesses seems to improve performance quite a bit

Code after:
  }

  async resolveAliases(
    filename: string,
    env: Env,
    pkg: InternalPackageJSON | null,
  ): Promise<?ResolvedAlias> {
    let localAliases = await this.resolvePackageAliases(filename, env, pkg);
    if (localAliases) {
      return localAliases;
    }

    // First resolve local package aliases, then project global ones.
    return this.resolvePackageAliases(filename, env, this.rootPackage);
  }

  async resolvePackageAliases(
    filename: string,
    env: Env,
    pkg: InternalPackageJSON | null,
  ): Promise<?ResolvedAlias> {
    if (!pkg) {
      return null;
    }

    if (pkg.source && !Array.isArray(pkg.source)) {
      let alias = await this.getAlias(filename, pkg, pkg.source);
      if (alias != null) {
        return alias;
      }
    }

    if (pkg.alias) {
      let alias = await this.getAlias(filename, pkg, pkg.alias);
      if (alias != null) {
        return alias;
      }
    }

    if (pkg.browser && env.isBrowser()) {
      let alias = await this.getAlias(filename, pkg, pkg.browser);
      if (alias != null) {
        return alias;
      }
    }

    return null;
  }

  async getAlias(
    filename: FilePath,
Repository: parcel-bundler/parcel
File: packages/core/utils/src/config.js (JavaScript)

Code before:
    if (cachedOutput) {
      return cachedOutput;
    }

    try {
      let extname = path.extname(configFile).slice(1);
      if (extname === 'js') {
        let output = {
          // $FlowFixMe
          config: clone(require(configFile)),
          files: [{filePath: configFile}],
        };

        configCache.set(configFile, output);
        return output;
      }

      let configContent = await fs.readFile(configFile, 'utf8');
      if (!configContent) return null;

      let config;
      if (parse === false) {
        config = configContent;
      } else {
        let parse = getParser(extname);
        config = parse(configContent);
      }

      let output = {
        config,
        files: [{filePath: configFile}],
      };

      configCache.set(String(parse) + configFile, output);
      return output;
    } catch (err) {
      if (err.code === 'MODULE_NOT_FOUND' || err.code === 'ENOENT') {
        return null;
      }

      throw err;
    }
  }

  return null;
}

loadConfig.clear = () => {
  configCache.reset();
  resolveCache.clear();
};

Review comment (performance):
That isn't part of the `configCache` cache key. But I also can't think of a case where you would want to parse a single file with 2 different parsers.

Code after:
    let cachedOutput = configCache.get(String(parse) + configFile);
    if (cachedOutput) {
      return cachedOutput;
    }

    try {
      let extname = path.extname(configFile).slice(1);
      if (extname === 'js') {
        let output = {
          // $FlowFixMe
          config: clone(require(configFile)),
          files: [{filePath: configFile}],
        };

        configCache.set(configFile, output);
        return output;
      }

      let configContent = await fs.readFile(configFile, 'utf8');
      if (!configContent) return null;

      let config;
      if (parse === false) {
        config = configContent;
      } else {
        let parse = opts?.parser ?? getParser(extname);
        config = parse(configContent);
      }

      let output = {
        config,
        files: [{filePath: configFile}],
      };

      configCache.set(String(parse) + configFile, output);
      return output;
    } catch (err) {
      if (err.code === 'MODULE_NOT_FOUND' || err.code === 'ENOENT') {
        return null;
      }

      throw err;
    }
  }

  return null;
}

loadConfig.clear = () => {
  configCache.reset();
  resolveCache.clear();
Repository: parcel-bundler/parcel
File: packages/core/core/src/requests/ParcelConfigRequest.js (JavaScript)

Code before:
      return {
        packageName: pkg,
        resolveFrom: filePath,
        keyPath: `${keyPath}/${i}`,
      };
    });
  }
}

function processMap(
  // $FlowFixMe
  map: ?ConfigMap<any, any>,
  keyPath: string,
  filePath: FilePath,
  // $FlowFixMe
): ConfigMap<any, any> | typeof undefined {
  if (!map) return undefined;

  // $FlowFixMe
  let res: ConfigMap<any, any> = {};
  for (let k in map) {
    if (k.startsWith('node:')) {
      throw new ThrowableDiagnostic({
        diagnostic: {
          message:
            'Named pipeline node: is reserved for builtin NodeJS libraries',
          origin: '@parcel/core',
          filePath,
        },
      });
    }

    if (typeof map[k] === 'string') {
      res[k] = {
        packageName: map[k],
        resolveFrom: filePath,
        keyPath: `${keyPath}/${k}`,
      };
    } else {
      res[k] = processPipeline(map[k], `${keyPath}/${k}`, filePath);
    }
  }

  return res;
}

export function processConfig(
  configFile: ResolvedParcelConfigFile,
): ProcessedParcelConfig {
  return {
    extends: configFile.extends,

Review comment (suggestion):
```suggestion
            'Named pipeline node: is reserved for builtin Node.js libraries',
```

Code after:
      return {
        packageName: pkg,
        resolveFrom: filePath,
        keyPath: `${keyPath}/${i}`,
      };
    });
  }
}

function processMap(
  // $FlowFixMe
  map: ?ConfigMap<any, any>,
  keyPath: string,
  filePath: FilePath,
  // $FlowFixMe
): ConfigMap<any, any> | typeof undefined {
  if (!map) return undefined;

  // $FlowFixMe
  let res: ConfigMap<any, any> = {};
  for (let k in map) {
    if (k.startsWith('node:')) {
      throw new ThrowableDiagnostic({
        diagnostic: {
          message:
            'Named pipeline node: is reserved for builtin Node.js libraries',
          origin: '@parcel/core',
          filePath,
        },
      });
    }

    if (typeof map[k] === 'string') {
      res[k] = {
        packageName: map[k],
        resolveFrom: filePath,
        keyPath: `${keyPath}/${k}`,
      };
    } else {
      res[k] = processPipeline(map[k], `${keyPath}/${k}`, filePath);
    }
  }

  return res;
}

export function processConfig(
  configFile: ResolvedParcelConfigFile,
): ProcessedParcelConfig {
  return {
    extends: configFile.extends,
Repository: parcel-bundler/parcel
File: packages/optimizers/cssnano/src/CSSNanoOptimizer.js (JavaScript)

Code before:
import postcss from 'postcss';
import cssnano from 'cssnano';
import type {CSSNanoOptions} from 'cssnano'; // TODO the type is based on cssnano 4
import path from 'path';

export default (new Optimizer({
  async loadConfig({config}) {
    const configFile = await config.getConfig(
      ['.cssnanorc', 'cssnano.config.json', 'cssnano.config.js'],
      {
        packageKey: 'cssnano',
      },
    );
    // TODO copied from stylus package. make this a utility function
    if (configFile) {
      let isJavascript = path.extname(configFile.filePath) === '.js';
      if (isJavascript) {
        config.shouldInvalidateOnStartup();
      }

      // Resolve relative paths from config file
      if (configFile.contents.paths) {
        configFile.contents.paths = configFile.contents.paths.map(p =>
          path.resolve(path.dirname(configFile.filePath), p),
        );
      }

      config.setResult(configFile.contents);
    }
  },

  async optimize({
    bundle,
    contents: prevContents,
    getSourceMapReference,
    map: prevMap,
    config,
    options,
  }) {
    if (!bundle.env.shouldOptimize) {
      return {contents: prevContents, map: prevMap};
    }

    if (typeof prevContents !== 'string') {
      throw new Error(
        'CSSNanoOptimizer: Only string contents are currently supported',
      );
    }

    const result = await postcss([
      cssnano((config ?? {}: CSSNanoOptions)),

Review comment (suggestion):
The only options in the config file are `plugins ` and `preset` https://cssnano.co/docs/config-file/

Code after:
import postcss from 'postcss';
import cssnano from 'cssnano';
import type {CSSNanoOptions} from 'cssnano'; // TODO the type is based on cssnano 4
import path from 'path';

export default (new Optimizer({
  async loadConfig({config}) {
    const configFile = await config.getConfig(
      ['.cssnanorc', 'cssnano.config.json', 'cssnano.config.js'],
      {
        packageKey: 'cssnano',
      },
    );
    if (configFile) {
      let isJavascript = path.extname(configFile.filePath) === '.js';
      if (isJavascript) {
        config.shouldInvalidateOnStartup();
      }
      config.setResult(configFile.contents);
    }
  },

  async optimize({
    bundle,
    contents: prevContents,
    getSourceMapReference,
    map: prevMap,
    config,
    options,
  }) {
    if (!bundle.env.shouldOptimize) {
      return {contents: prevContents, map: prevMap};
    }

    if (typeof prevContents !== 'string') {
      throw new Error(
        'CSSNanoOptimizer: Only string contents are currently supported',
      );
    }

    const result = await postcss([
      cssnano((config ?? {}: CSSNanoOptions)),
    ]).process(prevContents, {
      // Suppress postcss's warning about a missing `from` property. In this
      // case, the input map contains all of the sources.
      from: undefined,
      map: {
        annotation: false,
        inline: false,
        prev: prevMap ? await prevMap.stringify({}) : null,
      },