ronantakizawa/codereview-bench huggingface dataset:
Repository: parcel-bundler/parcel
File: packages/transformers/webextension/src/schema.js (JavaScript)

Code before:
        include_globs: arrStr,
        exclude_globs: arrStr,
        run_at: {
          type: 'string',
          enum: ['document_idle', 'document_start', 'document_end'],
        },
        all_frames: boolean,
      },
      additionalProperties: false,
      required: ['matches'],
    },
  },
  declarative_net_request: {
    type: 'object',
    properties: {
      rule_resources: {
        type: 'array',
        items: {
          type: 'object',
          properties: {
            id: string,
            enabled: boolean,
            path: string,
          },
          additionalProperties: false,
          required: ['id', 'enabled', 'path'],
        },
      },
      additionalProperties: false,
    },
  },
  devtools_page: string,
  // looks to be FF only
  dictionaries: ({
    type: 'object',
    properties: {},
    additionalProperties: string,
  }: SchemaEntity),
  externally_connectable: {
    type: 'object',
    properties: {
      ids: arrStr,
      matches: arrStr,
      accept_tls_channel_id: boolean,
    },
    additionalProperties: false,
  },
  // These next two are where it gets a bit Chrome-y
  // (we don't include all because some have next to no actual use)
  file_browser_handlers: {
    type: 'array',

Review comment (suggestion):
Are all these fields really required? Specifically, is the `enabled` field not true by default? I haven't used `declarativeNetRequest` before so forgive my lack of knowledge.

Code after:
        include_globs: arrStr,
        exclude_globs: arrStr,
        run_at: {
          type: 'string',
          enum: ['document_idle', 'document_start', 'document_end'],
        },
        all_frames: boolean,
      },
      additionalProperties: false,
      required: ['matches'],
    },
  },
  declarative_net_request: ({
    type: 'object',
    properties: {
      rule_resources: {
        type: 'array',
        items: {
          type: 'object',
          properties: {
            id: string,
            enabled: boolean,
            path: string,
          },
          additionalProperties: false,
          required: ['id', 'enabled', 'path'],
        },
      },
    },
    additionalProperties: false,
    required: ['rule_resources'],
  }: SchemaEntity),
  devtools_page: string,
  // looks to be FF only
  dictionaries: ({
    type: 'object',
    properties: {},
    additionalProperties: string,
  }: SchemaEntity),
  externally_connectable: {
    type: 'object',
    properties: {
      ids: arrStr,
      matches: arrStr,
      accept_tls_channel_id: boolean,
    },
    additionalProperties: false,
  },
  // These next two are where it gets a bit Chrome-y
  // (we don't include all because some have next to no actual use)
  file_browser_handlers: {
Repository: parcel-bundler/parcel
File: packages/core/integration-tests/test/scope-hoisting.js (JavaScript)

Code before:
      outputFS: slowFooFS,
      shouldDisableCache: true,
      workerFarm: workerFarmZeroConcurrent,
    });

    let bundleHashDelayFoo = b
      .getBundles()
      .find(b => b.filePath.endsWith('.js') && b.filePath.includes('index'))
      .filePath.split('.')[1];

    let slowBarFS = new Proxy(overlayFS, waitHandler('bar.js', 'foo.js'));

    let b2 = await bundle(path.join(testDir, 'index.html'), {
      inputFS: slowBarFS,
      outputFS: slowBarFS,
      shouldDisableCache: true,
      workerFarm: workerFarmZeroConcurrent,
    });

    let bundleHashDelayBar = b2
      .getBundles()
      .find(b => b.filePath.endsWith('.js') && b.filePath.includes('index'))
      .filePath.split('.')[1];

    assert.strictEqual(bundleHashDelayFoo, bundleHashDelayBar);
    await workerFarmZeroConcurrent.end();
  });
});


Review comment (bug):
Having a global `workerFarmZeroConcurrent` and then ending it in one test is weird, so it would be better to keep it as a local variable here. And also there should be a `try {..} finally { end(); }` around it in case the test fails.

---
The only other explanation I have for the CI error is that the `inputFS` memoryfs instance (and therefore also `overlayFS`) use a different worker farm than the one used in the `bundle()`:
https://github.com/parcel-bundler/parcel/blob/a9193ce0b19e93415f264b3295ceee093ddeb8c7/packages/core/test-utils/src/utils.js#L39-L40
So maybe try creating a new temporary `outputFS` and `overlayFS` but using the nonconcurrent farm.

Code after:
    let slowFooFS = new Proxy(overlayFS, waitHandler('foo.js', 'bar.js'));

    try {
      let b = await bundle(path.join(testDir, 'index.html'), {
        inputFS: slowFooFS,
        outputFS: slowFooFS,
        shouldDisableCache: true,
        workerFarm,
      });

      let bundleHashDelayFoo = b
        .getBundles()
        .find(b => b.filePath.endsWith('.js') && b.filePath.includes('index'))
        .filePath.split('.')[1];

      let slowBarFS = new Proxy(overlayFS, waitHandler('bar.js', 'foo.js'));

      let b2 = await bundle(path.join(testDir, 'index.html'), {
        inputFS: slowBarFS,
        outputFS: slowBarFS,
        shouldDisableCache: true,
        workerFarm,
      });

      let bundleHashDelayBar = b2
        .getBundles()
        .find(b => b.filePath.endsWith('.js') && b.filePath.includes('index'))
        .filePath.split('.')[1];

      assert.strictEqual(bundleHashDelayFoo, bundleHashDelayBar);
    } finally {
      await workerFarm.end();
    }
  });
});
Repository: parcel-bundler/parcel
File: packages/core/utils/src/config.js (JavaScript)

Code before:
  fs: FileSystem,
  filepath: FilePath,
  filenames: Array<FilePath>,
  projectRoot: FilePath,
): ?FilePath {
  return fs.findAncestorFile(filenames, path.dirname(filepath), projectRoot);
}

export async function loadConfig(
  fs: FileSystem,
  filepath: FilePath,
  filenames: Array<FilePath>,
  projectRoot: FilePath,
  opts: ?ConfigOptions,
): Promise<ConfigOutput | null> {
  let parse = opts?.parse ?? true;
  let configFile = await resolveConfig(fs, filepath, filenames, projectRoot);
  if (configFile) {
    let cachedOutput = configCache.get(String(parse) + configFile);
    if (cachedOutput) {
      return cachedOutput;
    }

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

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

      let configContent = await fs.readFile(configFile, 'utf8');

      let config;
      if (parse === false) {
        config = configContent;
      } else {
        let parse = opts?.parser ?? getParser(extname);
        try {
          config = parse(configContent);
        } catch (e) {
          if (extname !== '' && extname !== 'json') {
            throw e;
          }

          let pos = {

Review comment (suggestion):
Could be slightly faster, avoids array allocation.

```suggestion
      if (extname === 'js' || extname === 'cjs') {
```

Code after:
  fs: FileSystem,
  filepath: FilePath,
  filenames: Array<FilePath>,
  projectRoot: FilePath,
): ?FilePath {
  return fs.findAncestorFile(filenames, path.dirname(filepath), projectRoot);
}

export async function loadConfig(
  fs: FileSystem,
  filepath: FilePath,
  filenames: Array<FilePath>,
  projectRoot: FilePath,
  opts: ?ConfigOptions,
): Promise<ConfigOutput | null> {
  let parse = opts?.parse ?? true;
  let configFile = await resolveConfig(fs, filepath, filenames, projectRoot);
  if (configFile) {
    let cachedOutput = configCache.get(String(parse) + configFile);
    if (cachedOutput) {
      return cachedOutput;
    }

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

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

      let configContent = await fs.readFile(configFile, 'utf8');

      let config;
      if (parse === false) {
        config = configContent;
      } else {
        let parse = opts?.parser ?? getParser(extname);
        try {
          config = parse(configContent);
        } catch (e) {
          if (extname !== '' && extname !== 'json') {
            throw e;
          }

          let pos = {
Repository: parcel-bundler/parcel
File: packages/core/core/src/requests/ConfigRequest.js (JavaScript)

Code before:
  ParcelOptions,
  InternalFileCreateInvalidation,
} from '../types';
import type {LoadedPlugin} from '../ParcelConfig';
import type {RunAPI} from '../RequestTracker';
import type {ProjectPath} from '../projectPath';

import {serializeRaw} from '../serializer.js';
import {PluginLogger} from '@parcel/logger';
import PluginOptions from '../public/PluginOptions';
import ThrowableDiagnostic, {errorToDiagnostic} from '@parcel/diagnostic';
import PublicConfig from '../public/Config';
import {optionsProxy} from '../utils';
import {getInvalidationHash} from '../assetUtils';
import {Hash} from '@parcel/hash';

export type PluginWithLoadConfig = {
  loadConfig?: ({|
    config: IConfig,
    options: IPluginOptions,
    logger: IPluginLogger,
  |}) => Async<mixed>,
  ...
};

export type PluginWithLoadConfigGlobalInfo = {
  loadConfig?: ({|
    config: IConfig,
    options: IPluginOptions,
    logger: IPluginLogger,
  |}) => Async<mixed>,
  loadBundleConfig?: ({|
    bundle: INamedBundle,
    bundleGraph: IBundleGraph<INamedBundle>,
    config: IConfig,
    options: IPluginOptions,
    logger: IPluginLogger,
  |}) => Async<mixed>,
  ...
};

export type ConfigRequest = {
  id: string,
  invalidateOnFileChange: Set<ProjectPath>,
  invalidateOnFileCreate: Array<InternalFileCreateInvalidation>,
  invalidateOnEnvChange: Set<string>,
  invalidateOnOptionChange: Set<string>,
  invalidateOnStartup: boolean,
  invalidateOnBuild: boolean,
  ...
};

Review comment (suggestion):
```suggestion
export type PluginWithBundleConfig = {
```

Code after:
  ParcelOptions,
  InternalFileCreateInvalidation,
} from '../types';
import type {LoadedPlugin} from '../ParcelConfig';
import type {RunAPI} from '../RequestTracker';
import type {ProjectPath} from '../projectPath';

import {serializeRaw} from '../serializer.js';
import {PluginLogger} from '@parcel/logger';
import PluginOptions from '../public/PluginOptions';
import ThrowableDiagnostic, {errorToDiagnostic} from '@parcel/diagnostic';
import PublicConfig from '../public/Config';
import {optionsProxy} from '../utils';
import {getInvalidationHash} from '../assetUtils';
import {Hash} from '@parcel/hash';

export type PluginWithLoadConfig = {
  loadConfig?: ({|
    config: IConfig,
    options: IPluginOptions,
    logger: IPluginLogger,
  |}) => Async<mixed>,
  ...
};

export type PluginWithBundleConfig = {
  loadConfig?: ({|
    config: IConfig,
    options: IPluginOptions,
    logger: IPluginLogger,
  |}) => Async<mixed>,
  loadBundleConfig?: ({|
    bundle: INamedBundle,
    bundleGraph: IBundleGraph<INamedBundle>,
    config: IConfig,
    options: IPluginOptions,
    logger: IPluginLogger,
  |}) => Async<mixed>,
  ...
};

export type ConfigRequest = {
  id: string,
  invalidateOnFileChange: Set<ProjectPath>,
  invalidateOnFileCreate: Array<InternalFileCreateInvalidation>,
  invalidateOnEnvChange: Set<string>,
  invalidateOnOptionChange: Set<string>,
  invalidateOnStartup: boolean,
  invalidateOnBuild: boolean,
  ...
};
Repository: parcel-bundler/parcel
File: packages/transformers/svg-react/src/SvgReactTransformer.js (JavaScript)

Code before:
// @flow

import {Transformer} from '@parcel/plugin';

import path from 'path';
import camelcase from 'camelcase';
import svgoPlugin from '@svgr/plugin-svgo';
import jsxPlugin from '@svgr/plugin-jsx';
import {transform} from '@svgr/core';

function getComponentName(filePath) {
  let validCharacters = /[^a-zA-Z0-9_-]/g;
  let name = path.parse(filePath).name.replace(validCharacters, '');
  return camelcase(name, {
    pascalCase: true,
  });
}

export default (new Transformer({
  async loadConfig({config}) {
    let {contents} =
      (await config.getConfig(['.svgrrc.json', '.svgrrc'])) || {};

    return contents;
  },
  async transform({asset, config}) {
    let code = await asset.getCode();
    let componentName = getComponentName(asset.filePath);

    const jsx = await transform(
      code,
      {},
      {
        caller: {
          name: '@parcel/transformer-svg-react',
          defaultPlugins: [svgoPlugin, jsxPlugin],
        },
        filePath: componentName,
      },
    );

    asset.type = config?.typescript ? 'tsx' : 'jsx';
    asset.bundleBehavior = null;
    asset.setCode(jsx);

    return [asset];
  },
}): Transformer);


Review comment (suggestion):
Nit: this might be easier to read:

```suggestion
    let conf = await config.getConfig(['.svgrrc.json', '.svgrrc'])
    return conf?.contents;
```

Code after:
// @flow

import {Transformer} from '@parcel/plugin';

import path from 'path';
import camelcase from 'camelcase';
import svgoPlugin from '@svgr/plugin-svgo';
import jsxPlugin from '@svgr/plugin-jsx';
import {transform} from '@svgr/core';

function getComponentName(filePath) {
  let validCharacters = /[^a-zA-Z0-9_-]/g;
  let name = path.parse(filePath).name.replace(validCharacters, '');
  return camelcase(name, {
    pascalCase: true,
  });
}

export default (new Transformer({
  async loadConfig({config}) {
    let conf = await config.getConfig(['.svgrrc.json', '.svgrrc']);
    return conf?.contents;
  },
  async transform({asset, config}) {
    let code = await asset.getCode();
    let componentName = getComponentName(asset.filePath);

    const jsx = await transform(
      code,
      {},
      {
        caller: {
          name: '@parcel/transformer-svg-react',
          defaultPlugins: [svgoPlugin, jsxPlugin],
        },
        filePath: componentName,
      },
    );

    asset.type = config?.typescript ? 'tsx' : 'jsx';
    asset.bundleBehavior = null;
    asset.setCode(jsx);

    return [asset];
  },
}): Transformer);
Repository: parcel-bundler/parcel
File: packages/core/integration-tests/test/svg-react.js (JavaScript)

Code before:
import assert from 'assert';
import {bundle, outputFS} from '@parcel/test-utils';
import path from 'path';

describe('svg-react', function () {
  it('should support transforming SVGs to react components', async function () {
    let b = await bundle(
      path.join(__dirname, '/integration/svg-react/react.js'),
      {
        defaultConfig: path.join(
          __dirname,
          'integration/custom-configs/.parcelrc-svg-react',
        ),
      },
    );

    let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf-8');
    assert(!file.includes('inkscape'));
    assert(file.includes('const SvgIcon ='));
    assert(file.includes('_react.createElement("svg"'));
  });
});

describe('svg-react-typescript', function () {
  it('should support transforming SVGs to typescript react components', async function () {
    let b = await bundle(
      path.join(__dirname, '/integration/svg-react-typescript/react.ts'),
      {
        defaultConfig: path.join(
          __dirname,
          'integration/custom-configs/.parcelrc-svg-react',
        ),
        defaultTargetOptions: {
          typescript: true,
        },
      },
    );
    let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf-8');
    let types = await outputFS.readFile(b.getBundles()[1].filePath, 'utf-8');

    assert(!file.includes('inkscape'));
    assert(file.includes('react.createElement("svg"'));
    assert(types.includes('const Icon: SVGRComponent'));
  });
});


Review comment (suggestion):
We usually have one top-level describe block per file
```suggestion

```

Code after:
import assert from 'assert';
import {bundle, outputFS} from '@parcel/test-utils';
import path from 'path';

describe('svg-react', function () {
  it('should support transforming SVGs to react components', async function () {
    let b = await bundle(
      path.join(__dirname, '/integration/svg-react/react.js'),
      {
        defaultConfig: path.join(
          __dirname,
          'integration/custom-configs/.parcelrc-svg-react',
        ),
      },
    );

    let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf-8');
    assert(!file.includes('inkscape'));
    assert(file.includes('const SvgIcon ='));
    assert(file.includes('_react.createElement("svg"'));
  });
  it('should support transforming SVGs to typescript react components', async function () {
    let b = await bundle(
      path.join(__dirname, '/integration/svg-react-typescript/react.ts'),
      {
        defaultConfig: path.join(
          __dirname,
          'integration/custom-configs/.parcelrc-svg-react',
        ),
      },
    );
    let file = await outputFS.readFile(b.getBundles()[0].filePath, 'utf-8');
    let types = await outputFS.readFile(b.getBundles()[1].filePath, 'utf-8');

    assert(!file.includes('inkscape'));
    assert(file.includes('react.createElement("svg"'));
    assert(types.includes('const Icon: SVGRComponent'));
  });
});
Repository: parcel-bundler/parcel
File: packages/core/integration-tests/test/html.js (JavaScript)

Code before:
    );

    assertBundles(b, [
      {
        name: 'index.html',
        assets: ['index.html'],
      },
      {
        name: 'logo.svg',
        assets: ['logo.svg'],
      },
      {
        name: 'logo.14afa1de.svg',
        assets: ['logo.svg'],
      },
    ]);

    let html = await outputFS.readFile(
      path.join(distDir, 'index.html'),
      'utf8',
    );
    assert(html.includes(`<meta name="msapplication-config" content="none">`));
    assert(html.includes(`<meta property="og:image" content="/logo.svg">`));
    assert(
      html.includes(
        `<meta name="msapplication-TileImage" content="/logo.14afa1de.svg">`,
      ),
    );
    assert(
      html.includes(
        `<meta name="twitter:image" content="https://parceljs.org/assets/logo.svg">`,
      ),
    );
  });

  it('should insert sibling CSS bundles for JS files in the HEAD', async function () {
    let b = await bundle(
      path.join(__dirname, '/integration/html-css/index.html'),
    );

    assertBundles(b, [
      {
        name: 'index.html',
        assets: ['index.html'],
      },
      {
        type: 'js',
        assets: ['index.js'],
      },
      {
        type: 'css',

Review comment (suggestion):
This has to be a regex check instead `/<meta name="msapplication-TileImage" content="/logo\.[0-9a-f]+\.svg">`/`

Code after:
    );

    assertBundles(b, [
      {
        name: 'index.html',
        assets: ['index.html'],
      },
      {
        name: 'logo.svg',
        assets: ['logo.svg'],
      },
      {
        type: 'png',
        assets: ['logo.png'],
      },
      {
        type: 'png',
        assets: ['logo.png'],
      },
    ]);

    let html = await outputFS.readFile(
      path.join(distDir, 'index.html'),
      'utf8',
    );
    assert(html.includes(`<meta name="msapplication-config" content="none">`));
    assert(html.includes(`<meta property="og:image" content="/logo.svg">`));
    assert(
      /<meta name="msapplication-TileImage" content="\/logo\.[0-9a-f]+\.png">/.test(
        html,
      ),
    );
    assert(
      /<meta name="msapplication-square70x70logo" content="\/logo\.[0-9a-f]+\.png">/.test(
        html,
      ),
    );
    assert(
      html.includes(
        `<meta name="twitter:image" content="https://parceljs.org/assets/logo.svg">`,
      ),
    );
  });

  it('should insert sibling CSS bundles for JS files in the HEAD', async function () {
    let b = await bundle(
      path.join(__dirname, '/integration/html-css/index.html'),
    );

    assertBundles(b, [
      {
Repository: parcel-bundler/parcel
File: packages/core/integration-tests/test/html.js (JavaScript)

Code before:
        assets: ['b.html'],
      },
      {
        name: 'c.html',
        type: 'html',
        assets: ['c.html'],
      },
      {
        type: 'js',
        assets: ['a.html', 'shared.js'],
      },
      {
        type: 'js',
        assets: ['b.html', 'shared.js'],
      },
      {
        type: 'js',
        assets: ['c.html', 'shared.js'],
      },
      {
        type: 'css',
        assets: ['other.css'],
      },
      {
        type: 'css',
        assets: ['shared.css', 'other.css'], //We've duplicated 'other' to maintain 1 css bundle per entry
      },
    ]);

    // Both HTML files should point to the sibling CSS file
    let html = await outputFS.readFile(path.join(distDir, 'a.html'), 'utf8');
    assert(/<link rel="stylesheet" href="\/a\.[a-z0-9]+\.css">/.test(html));

    html = await outputFS.readFile(path.join(distDir, 'b.html'), 'utf8');
    assert(/<link rel="stylesheet" href="\/a\.[a-z0-9]+\.css">/.test(html));

    html = await outputFS.readFile(path.join(distDir, 'c.html'), 'utf8');
    assert(/<link rel="stylesheet" href="\/a\.[a-z0-9]+\.css">/.test(html));
  });

  it('should insert JS sibling bundle script tags in the correct order', async function () {
    let b = await bundle(
      path.join(
        __dirname,
        'integration/scope-hoisting/es6/interop-async/index.html',
      ),
      {
        mode: 'production',
        defaultTargetOptions: {
          shouldScopeHoist: true,
          shouldOptimize: false,

Review comment (suggestion):
Shouldn't this only apply to bundles with `needsStableName`? In this case, `other.css` is moved into a shared bundle which is fine because it's in HTML and not an entry.

Code after:
        assets: ['index.html'],
      },
      {
        assets: ['a.js', 'bundle-manifest.js', 'esm-js-loader.js'],
      },
      {
        assets: [
          'a.js',
          'bundle-manifest.js',
          'bundle-url.js',
          'cacheLoader.js',
          'js-loader.js',
        ],
      },
      {
        assets: [
          'b.js',
          'bundle-manifest.js',
          'bundle-url.js',
          'cacheLoader.js',
          'js-loader.js',
        ],
      },
      {
        assets: ['c.js'],
      },
      {
        assets: ['c.js'],
      },
    ]);

    let output = [];
    await run(b, {
      output(o) {
        output.push(o);
      },
    });

    // could run in either order.
    assert(output.sort(), ['a', 'b', 'c']);
  });

  it('should support multiple entries with shared sibling bundles', async function () {
    let b = await bundle(
      path.join(__dirname, '/integration/shared-sibling-entries/*.html'),
      {
        defaultTargetOptions: {
          shouldScopeHoist: true,
        },
      },
    );
Repository: parcel-bundler/parcel
File: packages/runtimes/js/src/helpers/bundle-manifest.js (JavaScript)

Code before:
var mapping = {};

function register(pairs) {
  var keys = Object.keys(pairs);
  for (var i = 0; i < keys.length; i++) {
    mapping[keys[i]] = pairs[keys[i]];
  }
}

function resolve(id) {
  var resolved = mapping[id];
  if (resolved == null) {
    throw new Error('Could not resolve bundle with id ' + id);
  }
  return resolved;
}

module.exports.register = register;
module.exports.resolve = resolve;


Review comment (suggestion):
This one definitely won't work in IE. Probably ok at this point? I guess we can't simply concatenate the strings? `path` could probably start with `../` or something...

Code after:
var mapping = new Map();

function register(baseUrl, manifest) {
  for (var i = 0; i < manifest.length - 1; i += 2) {
    mapping.set(manifest[i], {
      baseUrl: baseUrl,
      path: manifest[i + 1],
    });
  }
}

function resolve(id) {
  var resolved = mapping.get(id);
  if (resolved == null) {
    throw new Error('Could not resolve bundle with id ' + id);
  }
  return new URL(resolved.path, resolved.baseUrl).toString();
}

module.exports.register = register;
module.exports.resolve = resolve;