ronantakizawa/codereview-bench huggingface dataset:
Repository: parcel-bundler/parcel
File: packages/reporters/cli/src/bundleReport.js (JavaScript)

Code before:
            };
          }),
        };
  let rows = [];

  for (let bundle of bundles) {
    // Add a row for the bundle
    rows.push([
      formatFilename(bundle.filePath || '', chalk.cyan.bold),
      chalk.bold(prettifySize(bundle.size, bundle.size > LARGE_BUNDLE_SIZE)),
      chalk.green.bold(prettifyTime(bundle.time)),
    ]);

    if (assetCount > 0) {
      let largestAssets = bundle.assets.slice(0, assetCount);
      for (let asset of largestAssets) {
        let columns: Array<string> = [
          asset == largestAssets[largestAssets.length - 1] ? '└── ' : '├── ',
          chalk.dim(prettifySize(asset.size)),
          chalk.dim(chalk.green(prettifyTime(asset.time))),
        ];

        if (asset.filePath !== '') {
          columns[0] += formatFilename(asset.filePath, chalk.reset);
        } else {
          columns[0] += 'Code from unknown sourcefile';
        }

        // Add a row for the asset.
        rows.push(columns);
      }

      if (bundle.assets.length > largestAssets.length) {
        rows.push([
          '└── ' +
            chalk.dim(
              `+ ${bundle.assets.length - largestAssets.length} more assets`,
            ),
        ]);
      }

      // If this isn't the last bundle, add an empty row before the next one
      if (bundle !== bundles[bundles.length - 1]) {
        rows.push([]);
      }
    }
  }

  // Render table
  writeOut('');
  table(COLUMNS, rows);

Review comment (suggestion):
```suggestion
          columns[0] += 'Code from unknown sourcefiles';
```

Code after:
            };
          }),
        };
  let rows = [];

  for (let bundle of bundles) {
    // Add a row for the bundle
    rows.push([
      formatFilename(bundle.filePath || '', chalk.cyan.bold),
      chalk.bold(prettifySize(bundle.size, bundle.size > LARGE_BUNDLE_SIZE)),
      chalk.green.bold(prettifyTime(bundle.time)),
    ]);

    if (assetCount > 0) {
      let largestAssets = bundle.assets.slice(0, assetCount);
      for (let asset of largestAssets) {
        let columns: Array<string> = [
          asset == largestAssets[largestAssets.length - 1] ? '└── ' : '├── ',
          chalk.dim(prettifySize(asset.size)),
          chalk.dim(chalk.green(prettifyTime(asset.time))),
        ];

        if (asset.filePath !== '') {
          columns[0] += formatFilename(asset.filePath, chalk.reset);
        } else {
          columns[0] += 'Code from unknown sourcefiles';
        }

        // Add a row for the asset.
        rows.push(columns);
      }

      if (bundle.assets.length > largestAssets.length) {
        rows.push([
          '└── ' +
            chalk.dim(
              `+ ${bundle.assets.length - largestAssets.length} more assets`,
            ),
        ]);
      }

      // If this isn't the last bundle, add an empty row before the next one
      if (bundle !== bundles[bundles.length - 1]) {
        rows.push([]);
      }
    }
  }

  // Render table
  writeOut('');
  table(COLUMNS, rows);
Repository: parcel-bundler/parcel
File: packages/core/core/src/UncommittedAsset.js (JavaScript)

Code before:
      this.mapBuffer != null &&
        mapKey != null &&
        this.options.cache.setBlob(mapKey, this.mapBuffer),
      astKey != null &&
        this.options.cache.setBlob(
          astKey,
          // $FlowFixMe
          v8.serialize(this.ast),
        ),
    ]);
    this.value.contentKey = contentKey;
    this.value.mapKey = mapKey;
    this.value.astKey = astKey;
    this.value.outputHash = md5FromString(
      [this.value.hash, pipelineKey].join(':'),
    );

    if (this.content != null) {
      this.value.stats.size = size;
    }

    this.value.committed = true;
  }

  // Please do not use this outside of uncomittedasset. Thanks :)
  async __INTERNAL_getContent(): Promise<string> {
    let content = await this.content;
    if (typeof content === 'string' || content instanceof Buffer) {
      return content.toString();
    } else if (content != null) {
      this.content = bufferStream(content);
      return (await this.content).toString();
    }

    return '';
  }

  getCode(): Promise<string> {
    if (this.ast != null && this.isASTDirty) {
      throw new Error(
        'Cannot call getCode() on an asset with a dirty AST. For transformers, implement canReuseAST() and check asset.isASTDirty.',
      );
    }

    return this.__INTERNAL_getContent();
  }

  async getBuffer(): Promise<Buffer> {
    let content = await this.content;
    if (content == null) {
      return Buffer.alloc(0);

Review comment (suggestion):
You call this in the same class, so I would make factor this out into `function getAssetContent(asset: UncommitedAsset) {...}` and call `getAssetContent(this) in `getCode` and `loadExistingSourcemap `

Code after:
        ),
      this.mapBuffer != null &&
        mapKey != null &&
        this.options.cache.setBlob(mapKey, this.mapBuffer),
      astKey != null &&
        this.options.cache.setBlob(
          astKey,
          // $FlowFixMe
          v8.serialize(this.ast),
        ),
    ]);
    this.value.contentKey = contentKey;
    this.value.mapKey = mapKey;
    this.value.astKey = astKey;
    this.value.outputHash = md5FromString(
      [this.value.hash, pipelineKey].join(':'),
    );

    if (this.content != null) {
      this.value.stats.size = size;
    }

    this.value.committed = true;
  }

  async getCode(): Promise<string> {
    if (this.ast != null && this.isASTDirty) {
      throw new Error(
        'Cannot call getCode() on an asset with a dirty AST. For transformers, implement canReuseAST() and check asset.isASTDirty.',
      );
    }

    let content = await this.content;
    if (typeof content === 'string' || content instanceof Buffer) {
      return content.toString();
    } else if (content != null) {
      this.content = bufferStream(content);
      return (await this.content).toString();
    }

    return '';
  }

  async getBuffer(): Promise<Buffer> {
    let content = await this.content;
    if (content == null) {
      return Buffer.alloc(0);
    } else if (typeof content === 'string' || content instanceof Buffer) {
      return Buffer.from(content);
    }
Repository: parcel-bundler/parcel
File: packages/core/utils/src/sourcemap.js (JavaScript)

Code before:
// @flow
import type {FileSystem} from '@parcel/fs';
import SourceMap from '@parcel/source-map';
import path from 'path';
import {normalizeSeparators} from './path';

const SOURCEMAP_RE = /(?:\/\*|\/\/)\s*[@#]\s*sourceMappingURL\s*=\s*([^\s*]+)(?:\s*\*\/)?\s*$/;
const DATA_URL_RE = /^data:[^;]+(?:;charset=[^;]+)?;base64,(.*)/;
export const SOURCEMAP_EXTENSIONS: Set<string> = new Set<string>([
  'js',
  'css',
  'scss',
  'sass',
  'styl',
]);

export function matchSourceMappingURL(
  contents: string,
): null | RegExp$matchResult {
  return contents.match(SOURCEMAP_RE);
}

export async function loadSourceMapUrl(
  fs: FileSystem,
  filename: string,
  contents: string,
): Promise<?{|filename: string, map: any, url: string|}> {
  let match = matchSourceMappingURL(contents);
  if (match) {
    let url = match[1].trim();
    let dataURLMatch = url.match(DATA_URL_RE);
    filename = dataURLMatch ? filename : path.join(path.dirname(filename), url);

    return {
      url,
      filename,
      map: JSON.parse(
        dataURLMatch
          ? Buffer.from(dataURLMatch[1], 'base64').toString()

Review comment (suggestion):
typescript? jsx?
Though I'm not happy about adding yet another extensions list (like the one in the resolver)

Code after:
// @flow
import type {FileSystem} from '@parcel/fs';
import SourceMap from '@parcel/source-map';
import path from 'path';
import {normalizeSeparators} from './path';

export const SOURCEMAP_RE: RegExp = /(?:\/\*|\/\/)\s*[@#]\s*sourceMappingURL\s*=\s*([^\s*]+)(?:\s*\*\/)?\s*$/;
const DATA_URL_RE = /^data:[^;]+(?:;charset=[^;]+)?;base64,(.*)/;
export const SOURCEMAP_EXTENSIONS: Set<string> = new Set<string>([
  'js',
  'jsx',
  'mjs',
  'es',
  'es6',
  'css',
]);

export function matchSourceMappingURL(
  contents: string,
): null | RegExp$matchResult {
  return contents.match(SOURCEMAP_RE);
}

export async function loadSourceMapUrl(
  fs: FileSystem,
  filename: string,
  contents: string,
): Promise<?{|filename: string, map: any, url: string|}> {
  let match = matchSourceMappingURL(contents);
  if (match) {
    let url = match[1].trim();
    let dataURLMatch = url.match(DATA_URL_RE);
    filename = dataURLMatch ? filename : path.join(path.dirname(filename), url);

    return {
      url,
      filename,
      map: JSON.parse(
        dataURLMatch
Repository: parcel-bundler/parcel
File: packages/transformers/typescript-types/src/TSTypesTransformer.js (JavaScript)

Code before:
          codeFrame: codeframe ? codeframe : undefined,
        });
      }
    }

    let code = nullthrows(host.outputCode);
    code = code.substring(0, code.lastIndexOf('//# sourceMappingURL'));

    let map = JSON.parse(nullthrows(host.outputMap));
    map.sources = map.sources.map(source =>
      path.join(path.dirname(asset.filePath), source),
    );

    let sourceMap = null;
    if (map.mappings) {
      sourceMap = new SourceMap(options.projectRoot);
      sourceMap.addRawMappings(map);
    }

    return [
      {
        type: 'ts',
        // Stay on the types pipeline, even if the type changes
        pipeline: asset.pipeline,
        content: code,
        map: sourceMap,
        includedFiles,
      },
    ];
  },
}): Transformer);


Review comment (bug):
Due to a bug in Transformation, this never surfaced, but changing an asset type from `tsx` to `ts` would also reset the pipeline, and this Typescript asset would then continue on the regular js pipeline.

Code after:
          message: diagnosticMessage,
          filePath: filename,
          codeFrame: codeframe ? codeframe : undefined,
        });
      }
    }

    let code = nullthrows(host.outputCode);
    code = code.substring(0, code.lastIndexOf('//# sourceMappingURL'));

    let map = JSON.parse(nullthrows(host.outputMap));
    map.sources = map.sources.map(source =>
      path.join(path.dirname(asset.filePath), source),
    );

    let sourceMap = null;
    if (map.mappings) {
      sourceMap = new SourceMap(options.projectRoot);
      sourceMap.addRawMappings(map);
    }

    return [
      {
        type: 'ts',
        // Stay on the types pipeline, even if the type changes
        pipeline: asset.pipeline,
        content: code,
        map: sourceMap,
        includedFiles,
      },
    ];
  },
}): Transformer);
Repository: parcel-bundler/parcel
File: packages/core/integration-tests/test/globals.js (JavaScript)

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

describe('global alias', function() {
  it('should support global alias syntax', async function() {
    let b = await bundle(
      path.join(__dirname, '/integration/global-alias/index.js'),
    );

    let index = await outputFS.readFile(
      b.getBundles().find(b => b.name.startsWith('index')).filePath,
      'utf8',
    );

    assert(/module\.exports\ =\ React/.test(index));
  });
});


Review comment (suggestion):
Testing this using  `module.exports = React.createElement...` and
```js
let output = await run(b, {React: {createElement(tag){ return "ok"; }}});

assert.equals(output, "ok");
```
would be more robust.

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

describe('global alias', function() {
  it('should support global alias syntax', async function() {
    let b = await bundle(
      path.join(__dirname, '/integration/global-alias/index.js'),
    );

    assert.equal(
      await run(b, {
        React: {
          createElement: function() {
            return 'ok';
          },
        },
      }),
      'ok',
    );
  });
});
Repository: parcel-bundler/parcel
File: packages/core/codeframe/test/codeframe.test.js (JavaScript)

Code before:
            line: 8,
            column: 48,
          },
        },
      ],
      {
        useColor: false,
        syntaxHighlighting: false,
        language: 'js',
        terminalWidth: 100,
      },
    );

    let lines = codeframeString.split(LINE_END);
    assert.equal(lines.length, 5);
    assert.equal(lines[0], '  7  |  */');
    assert.equal(
      lines[1],
      `> 8  | import { unstable_CompositeItem as CompositeItem } from 'reakit/Composite';`,
    );
    assert.equal(
      lines[2],
      '>    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^',
    );
    assert.equal(lines[3], '  9  | /**');
    assert.equal(lines[4], '  10 |  * Internal dependencies');
  });
});


Review comment (suggestion):
Line numbers are usually right-aligned:
```
 9 | /**
10 |  * Internal dependencies
```

<img width="252" alt="Bildschirmfoto 2020-07-11 um 17 50 35" src="https://user-images.githubusercontent.com/4586894/87228063-105d7c80-c39f-11ea-8e4f-a6c067fa5e78.png">


Code after:
            line: 8,
            column: 48,
          },
        },
      ],
      {
        useColor: false,
        syntaxHighlighting: false,
        language: 'js',
        terminalWidth: 100,
      },
    );

    let lines = codeframeString.split(LINE_END);
    assert.equal(lines.length, 5);
    assert.equal(lines[0], `   7 | import Tooltip from '../tooltip';`);
    assert.equal(
      lines[1],
      `>  8 | import VisuallyHidden from '../visually-hidden';`,
    );
    assert.equal(
      lines[2],
      '>    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^',
    );
    assert.equal(lines[3], '   9 | ');
    assert.equal(lines[4], '  10 | /**');
  });
});
Repository: parcel-bundler/parcel
File: packages/utils/node-resolver-core/src/NodeResolver.js (JavaScript)

Code before:
const QUERY_PARAMS_REGEX = /^([^\t\r\n\v\f?]*)(\?.*)?/;

type InternalPackageJSON = PackageJSON & {pkgdir: string, pkgfile: string, ...};
type Options = {|
  options: PluginOptions,
  extensions: Array<string>,
  mainFields: Array<string>,
|};
type ResolvedFile = {|
  path: string,
  pkg: InternalPackageJSON | null,
|};

type Aliases =
  | string
  | {[string]: string, ...}
  | {[string]: string | boolean, ...};
type Module = {|
  moduleName?: string,
  subPath?: ?string,
  moduleDir?: FilePath,
  filePath?: FilePath,
  code?: string,
|};

const parseFilePath = (
  filepath: FilePath,
): {|
  filepath: FilePath,
  query: QueryParameters,
|} => {
  let matches = filepath.match(QUERY_PARAMS_REGEX);
  if (matches && matches.length > 2) {
    return {
      filepath: matches[1],
      query: matches[2] ? querystring.parse(matches[2].substr(1)) : {},
    };
  }

  return {
    filepath,
    query: {},
  };
};

/**
 * This resolver implements a modified version of the node_modules resolution algorithm:
 * https://nodejs.org/api/modules.html#modules_all_together
 *
 * In addition to the standard algorithm, Parcel supports:
 *   - All file extensions supported by Parcel.

Review comment (suggestion):
Wonder which parts of specifier parsing should be up to the resolver and which should be up to core. I guess the pipeline is parsed by core. Should query params be parsed by core as well? No strong opinion just wondering.

Code after:
type Options = {|
  options: PluginOptions,
  extensions: Array<string>,
  mainFields: Array<string>,
|};
type ResolvedFile = {|
  path: string,
  pkg: InternalPackageJSON | null,
|};

type Aliases =
  | string
  | {[string]: string, ...}
  | {[string]: string | boolean, ...};
type Module = {|
  moduleName?: string,
  subPath?: ?string,
  moduleDir?: FilePath,
  filePath?: FilePath,
  code?: string,
|};

/**
 * This resolver implements a modified version of the node_modules resolution algorithm:
 * https://nodejs.org/api/modules.html#modules_all_together
 *
 * In addition to the standard algorithm, Parcel supports:
 *   - All file extensions supported by Parcel.
 *   - Glob file paths
 *   - Absolute paths (e.g. /foo) resolved relative to the project root.
 *   - Tilde paths (e.g. ~/foo) resolved relative to the nearest module root in node_modules.
 *   - The package.json module, jsnext:main, and browser field as replacements for package.main.
 *   - The package.json browser and alias fields as an alias map within a local module.
 *   - The package.json alias field in the root package for global aliases across all modules.
 */
export default class NodeResolver {
  options: PluginOptions;
  extensions: Array<string>;
  mainFields: Array<string>;
  packageCache: Map<string, InternalPackageJSON>;
  rootPackage: InternalPackageJSON | null;

  constructor(opts: Options) {
    this.extensions = opts.extensions.map(ext =>
      ext.startsWith('.') ? ext : '.' + ext,
    );
    this.mainFields = opts.mainFields;
    this.options = opts.options;
    this.packageCache = new Map();
    this.rootPackage = null;
  }
Repository: parcel-bundler/parcel
File: packages/core/core/src/BundleGraph.js (JavaScript)

Code before:

  getBundleGroupsContainingBundle(bundle: Bundle): Array<BundleGroup> {
    return this._graph
      .getNodesConnectedTo(nullthrows(this._graph.getNode(bundle.id)), 'bundle')
      .filter(node => node.type === 'bundle_group')
      .map(node => {
        invariant(node.type === 'bundle_group');
        return node.value;
      });
  }

  getBundlesInBundleGroup(bundleGroup: BundleGroup): Array<Bundle> {
    return this._graph
      .getNodesConnectedFrom(
        nullthrows(this._graph.getNode(getBundleGroupId(bundleGroup))),
        'bundle',
      )
      .filter(node => node.type === 'bundle')
      .map(node => {
        invariant(node.type === 'bundle');
        return node.value;
      });
  }

  getSiblingBundles(bundle: Bundle): Array<Bundle> {
    let siblings = new Set();

    let bundleGroups = this.getBundleGroupsContainingBundle(bundle);
    for (let bundleGroup of bundleGroups) {
      let bundles = this.getBundlesInBundleGroup(bundleGroup);
      for (let b of bundles) {
        if (b.id !== bundle.id) {
          siblings.add(b);
        }
      }
    }

    return [...siblings];
  }

  getReferencedBundles(bundle: Bundle): Array<Bundle> {
    return this._graph
      .getNodesConnectedFrom(
        nullthrows(this._graph.getNode(bundle.id)),
        'references',
      )
      .filter(node => node.type === 'bundle')
      .map(node => {
        invariant(node.type === 'bundle');
        return node.value;
      });

Review comment (suggestion):
Should that function return a set instead?
But if every callsite does `.map` or filter anyway it's more convenient as it is...

Code after:
  }

  getBundleGroupsContainingBundle(bundle: Bundle): Array<BundleGroup> {
    return this._graph
      .getNodesConnectedTo(nullthrows(this._graph.getNode(bundle.id)), 'bundle')
      .filter(node => node.type === 'bundle_group')
      .map(node => {
        invariant(node.type === 'bundle_group');
        return node.value;
      });
  }

  getBundlesInBundleGroup(bundleGroup: BundleGroup): Array<Bundle> {
    return this._graph
      .getNodesConnectedFrom(
        nullthrows(this._graph.getNode(getBundleGroupId(bundleGroup))),
        'bundle',
      )
      .filter(node => node.type === 'bundle')
      .map(node => {
        invariant(node.type === 'bundle');
        return node.value;
      });
  }

  getSiblingBundles(bundle: Bundle): Array<Bundle> {
    let siblings = new Set();

    let bundleGroups = this.getBundleGroupsContainingBundle(bundle);
    for (let bundleGroup of bundleGroups) {
      let bundles = this.getBundlesInBundleGroup(bundleGroup);
      for (let b of bundles) {
        if (b.id !== bundle.id) {
          siblings.add(b);
        }
      }
    }

    return [...siblings];
  }

  getReferencedBundles(bundle: Bundle): Array<Bundle> {
    return this._graph
      .getNodesConnectedFrom(
        nullthrows(this._graph.getNode(bundle.id)),
        'references',
      )
      .filter(node => node.type === 'bundle')
      .map(node => {
        invariant(node.type === 'bundle');
        return node.value;
Repository: parcel-bundler/parcel
File: packages/transformers/js/src/JSTransformer.js (JavaScript)

Code before:
      !asset.env.scopeHoist &&
      !canHaveDependencies(code) &&
      !ENV_RE.test(code) &&
      !BROWSER_RE.test(code) &&
      !FS_RE.test(code)
    ) {
      return null;
    }

    return parse({
      asset,
      code,
      options,
    });
  },

  async transform({asset, config, options, logger}) {
    // When this asset is an bundle entry, allow that bundle to be split to load shared assets separately.
    // Only set here if it is null to allow previous transformers to override this behavior.
    if (asset.isSplittable == null) {
      asset.isSplittable = true;
    }

    asset.type = 'js';
    let ast = await asset.getAST();
    if (!ast) {
      return [asset];
    }

    let {inlineEnvironment = true, inlineFS = true} = config || {};

    let code = asset.isASTDirty() ? null : await asset.getCode();

    // Inline process/environment variables
    if (
      (!asset.env.isNode() && (code == null || ENV_RE.test(code))) ||
      (asset.env.isBrowser() && (code == null || BROWSER_RE.test(code)))
    ) {
      walkAncestor(ast.program, processVisitor, {
        asset,
        ast,
        env: options.env,
        isNode: asset.env.isNode(),
        replaceEnv: inlineEnvironment,
        isBrowser: asset.env.isBrowser(),
      });
    }

    let isASTDirty;
    if (!asset.env.isNode()) {
      // Inline fs calls, run before globals to also collect Buffer

Review comment (suggestion):
Did we want to change the defaults here? I thought we had decided to disable environment inlining (except `NODE_ENV`), and `fs` by default.

Code after:
      !FS_RE.test(code)
    ) {
      return null;
    }

    return parse({
      asset,
      code,
      options,
    });
  },

  async transform({asset, config, options, logger}) {
    // When this asset is an bundle entry, allow that bundle to be split to load shared assets separately.
    // Only set here if it is null to allow previous transformers to override this behavior.
    if (asset.isSplittable == null) {
      asset.isSplittable = true;
    }

    asset.type = 'js';
    let ast = await asset.getAST();
    if (!ast) {
      return [asset];
    }

    let {inlineEnvironment = true, inlineFS = true} = config || {};

    let code = asset.isASTDirty() ? null : await asset.getCode();

    // Inline process/environment variables
    if (
      (!asset.env.isNode() && (code == null || ENV_RE.test(code))) ||
      (asset.env.isBrowser() && (code == null || BROWSER_RE.test(code)))
    ) {
      walkAncestor(ast.program, processVisitor, {
        asset,
        ast,
        env: options.env,
        isNode: asset.env.isNode(),
        replaceEnv: inlineEnvironment,
        isBrowser: asset.env.isBrowser(),
      });
    }

    let isASTDirty;
    if (!asset.env.isNode()) {
      // Inline fs calls, run before globals to also collect Buffer
      if (code == null || FS_RE.test(code)) {
        // Check if we should ignore fs calls
        // See https://github.com/defunctzombie/node-browser-resolve#skip
        let pkg = await asset.getPackage();