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

Code before:
// @flow
import {Transformer} from '@parcel/plugin';

import {parse, print, Source} from 'graphql/language';
import {stripIgnoredCharacters} from 'graphql/utilities';
import {processDocumentImports} from 'graphql-import-macro';

export default new Transformer({
  async transform({asset, options, resolve}) {
    const document = parse(new Source(await asset.getCode(), asset.filePath));

    const expandedDocument = await processDocumentImports(document, loadImport);

    async function loadImport(to, from) {
      const filePath = await resolve(to, from);

      asset.addIncludedFile({filePath});

      return parse(
        new Source(await options.inputFS.readFile(filePath, 'utf-8'), filePath),
      );
    }

    const generated = asset.env.minify
      ? stripIgnoredCharacters(print(expandedDocument))
      : print(expandedDocument);

    asset.type = 'js';
    asset.setCode(`module.exports=${JSON.stringify(generated)};`);

    return [asset];

Review comment (suggestion):
You need to use `options.packageManager.require` (just like the old graphql transformer did) because these two dependencies are not a `package.json#dependency` (and `graphql` is quite big to install). This way it will be autoinstalled into the users project on the first usage.

Code after:
// @flow
import {Transformer} from '@parcel/plugin';

export default new Transformer({
  async transform({asset, options, resolve}) {
    const {
      parse,
      print,
      Source,
      stripIgnoredCharacters,
    } = await options.packageManager.require('graphql', asset.filePath, {
      autoinstall: options.autoinstall,
    });

    const {processDocumentImports} = await options.packageManager.require(
      'graphql-import-macro',
      asset.filePath,
      {
        autoinstall: options.autoinstall,
      },
    );

    const document = parse(new Source(await asset.getCode(), asset.filePath));

    const expandedDocument = await processDocumentImports(document, loadImport);

    async function loadImport(to, from) {
      const filePath = await resolve(to, from);

      asset.addIncludedFile({filePath});
Repository: parcel-bundler/parcel
File: packages/core/codeframe/src/codeframe.js (JavaScript)

Code before:
// @flow
import type {DiagnosticCodeHighlight} from '@parcel/diagnostic';

import chalk from 'chalk';
import emphasize from 'emphasize';
import stringWidth from 'string-width';
import sliceAnsi from 'slice-ansi';

type CodeFramePadding = {|
  before: number,
  after: number,
|};

type CodeFrameOptionsInput = {
  useColor?: boolean,
  maxLines?: number,
  padding?: CodeFramePadding,
  syntaxHighlighting?: boolean,
  language?: string,
  terminalWidth?: number,
  ...
};

type CodeFrameOptions = {|
  useColor: boolean,
  syntaxHighlighting: boolean,
  maxLines: number,
  padding: CodeFramePadding,
  terminalWidth: number,
  language?: string,
|};

const NEWLINE = /\r\n|[\n\r\u2028\u2029]/;
const TAB_REPLACE_REGEX = /\t/g;
const TAB_REPLACEMENT = '  ';
const DEFAULT_TERMINAL_WIDTH = 80;

const highlightSyntax = (txt: string, lang?: string): string => {
  if (lang) {

Review comment (suggestion):
`type CodeFrameOptionsInput = $Shape<CodeFrameOptions>;`


Code after:
// @flow
import type {DiagnosticCodeHighlight} from '@parcel/diagnostic';

import chalk from 'chalk';
import emphasize from 'emphasize';
import stringWidth from 'string-width';
import sliceAnsi from 'slice-ansi';

type CodeFramePadding = {|
  before: number,
  after: number,
|};

type CodeFrameOptionsInput = $Shape<CodeFrameOptions>;

type CodeFrameOptions = {|
  useColor: boolean,
  syntaxHighlighting: boolean,
  maxLines: number,
  padding: CodeFramePadding,
  terminalWidth: number,
  language?: string,
|};

const NEWLINE = /\r\n|[\n\r\u2028\u2029]/;
const TAB_REPLACE_REGEX = /\t/g;
const TAB_REPLACEMENT = '  ';
const DEFAULT_TERMINAL_WIDTH = 80;

const highlightSyntax = (txt: string, lang?: string): string => {
  if (lang) {
    try {
      return emphasize.highlight(lang, txt).value;
    } catch (e) {
      // fallback for unknown languages...
    }
  }

  return emphasize.highlightAuto(txt).value;
Repository: parcel-bundler/parcel
File: packages/core/codeframe/src/codeframe.js (JavaScript)

Code before:
const DEFAULT_TERMINAL_WIDTH = 80;

const highlightSyntax = (txt: string, lang?: string): string => {
  if (lang) {
    try {
      return emphasize.highlight(lang, txt).value;
    } catch (e) {
      // fallback for unknown languages...
    }
  }

  return emphasize.highlightAuto(txt).value;
};

export default function codeFrame(
  code: string,
  highlights: Array<DiagnosticCodeHighlight>,
  inputOpts: CodeFrameOptionsInput = {},
): string {
  if (highlights.length < 1) return '';

  let opts: CodeFrameOptions = {
    useColor: !!inputOpts.useColor,
    syntaxHighlighting: !!inputOpts.syntaxHighlighting,
    language: inputOpts.language,
    maxLines: inputOpts.maxLines !== undefined ? inputOpts.maxLines : 12,
    terminalWidth: inputOpts.terminalWidth || DEFAULT_TERMINAL_WIDTH,
    padding: inputOpts.padding || {
      before: 1,
      after: 2,
    },
  };

  // Highlights messages and prefixes when colors are enabled
  const highlighter = (s: string, bold?: boolean) => {
    if (opts.useColor) {
      let redString = chalk.red(s);
      return bold ? chalk.bold(redString) : redString;
    }

    return s;
  };

  // Prefix lines with the line number
  const lineNumberPrefixer = (params: {|
    lineNumber?: string,
    endLine: string,
    isHighlighted: boolean,
  |}) => {
    let {lineNumber, endLine, isHighlighted} = params;


Review comment (suggestion):
```suggestion
    maxLines: inputOpts.maxLines ?? 12,
```

Code after:
    }
  }

  return emphasize.highlightAuto(txt).value;
};

export default function codeFrame(
  code: string,
  highlights: Array<DiagnosticCodeHighlight>,
  inputOpts: CodeFrameOptionsInput = {},
): string {
  if (highlights.length < 1) return '';

  let opts: CodeFrameOptions = {
    useColor: !!inputOpts.useColor,
    syntaxHighlighting: !!inputOpts.syntaxHighlighting,
    language: inputOpts.language,
    maxLines: inputOpts.maxLines ?? 12,
    terminalWidth: inputOpts.terminalWidth || DEFAULT_TERMINAL_WIDTH,
    padding: inputOpts.padding || {
      before: 1,
      after: 2,
    },
  };

  // Highlights messages and prefixes when colors are enabled
  const highlighter = (s: string, bold?: boolean) => {
    if (opts.useColor) {
      let redString = chalk.red(s);
      return bold ? chalk.bold(redString) : redString;
    }

    return s;
  };

  // Prefix lines with the line number
  const lineNumberPrefixer = (params: {|
    lineNumber?: string,
    endLine: string,
    isHighlighted: boolean,
  |}) => {
    let {lineNumber, endLine, isHighlighted} = params;

    return `${isHighlighted ? highlighter('>') : ' '} ${
      lineNumber
        ? lineNumber.padEnd(endLine.length, ' ')
        : ' '.repeat(endLine.length)
    } | `;
  };

  // Make columns/lines start at 1
Repository: parcel-bundler/parcel
File: packages/core/core/test/TargetResolver.test.js (JavaScript)

Code before:
              },
              includeNodeModules: false,
              outputFormat: 'commonjs',
              isLibrary: true,
              minify: false,
              scopeHoist: false,
            },
            sourceMap: {},
            loc: {
              filePath: path.join(COMMON_TARGETS_FIXTURE_PATH, 'package.json'),
              start: {
                column: 14,
                line: 4,
              },
              end: {
                column: 36,
                line: 4,
              },
            },
          },
        ],
      },
    );
  });

  it('generates a default target in server mode', async () => {
    let targetResolver = new TargetResolver({
      ...DEFAULT_OPTIONS,
      serve: {port: 1234},
    });

    assert.deepEqual(
      await targetResolver.resolve(COMMON_TARGETS_FIXTURE_PATH),
      {
        targets: [
          {
            name: 'default',
            distDir: DEFAULT_OPTIONS.distDir,
            publicUrl: '/',
            env: {
              context: 'browser',
              engines: {
                browsers: [
                  'last 1 Chrome version',
                  'last 1 Safari version',
                  'last 1 Firefox version',
                  'last 1 Edge version',
                ],
              },
              includeNodeModules: true,
              outputFormat: 'global',

Review comment (suggestion):
```suggestion
  it('generates a default target in serve mode', async () => {
```

there's a typo in the title, I think

Code after:
              },
              includeNodeModules: false,
              outputFormat: 'commonjs',
              isLibrary: true,
              minify: false,
              scopeHoist: false,
            },
            sourceMap: {},
            loc: {
              filePath: path.join(COMMON_TARGETS_FIXTURE_PATH, 'package.json'),
              start: {
                column: 14,
                line: 4,
              },
              end: {
                column: 36,
                line: 4,
              },
            },
          },
        ],
      },
    );
  });

  it('generates a default target in serve mode', async () => {
    let targetResolver = new TargetResolver({
      ...DEFAULT_OPTIONS,
      serve: {port: 1234},
    });

    assert.deepEqual(
      await targetResolver.resolve(COMMON_TARGETS_FIXTURE_PATH),
      {
        targets: [
          {
            name: 'default',
            distDir: DEFAULT_OPTIONS.distDir,
            publicUrl: '/',
            env: {
              context: 'browser',
              engines: {
                browsers: [
                  'last 1 Chrome version',
                  'last 1 Safari version',
                  'last 1 Firefox version',
                  'last 1 Edge version',
                ],
              },
              includeNodeModules: true,
              outputFormat: 'global',
Repository: parcel-bundler/parcel
File: packages/core/core/src/public/Symbols.js (JavaScript)

Code before:
    this.#value = asset;
    valueToSymbols.set(asset, this);
  }

  get(exportSymbol: Symbol): ?{|local: Symbol, loc: ?SourceLocation|} {
    return this.#value.symbols?.get(exportSymbol);
  }

  hasExportSymbol(exportSymbol: Symbol): boolean {
    return Boolean(this.#value.symbols?.has(exportSymbol));
  }

  hasLocalSymbol(local: Symbol): boolean {
    if (this.#value.symbols) {
      for (let s of this.#value.symbols.values()) {
        if (local === s.local) return true;
      }
    }
    return false;
  }

  // $FlowFixMe
  [Symbol.iterator]() {
    return this.#value.symbols
      ? this.#value.symbols[Symbol.iterator]()
      : {
          next() {
            return {done: true};
          },
        };
  }

  get isCleared() {
    return this.#value.symbols == null;
  }
}

let valueToMutableSymbols: WeakMap<
  Asset | Dependency,
  MutableSymbols,
> = new WeakMap();

class MutableSymbols {
  /*::
  @@iterator(): Iterator<[Symbol, {|local: Symbol, loc: ?SourceLocation|}]> { return ({}: any); }
  */
  #value; // Asset

  constructor(asset: Asset | Dependency) {
    this.#value = asset;
  }

Review comment (suggestion):
Maybe turn this into a function called `createEmptyIterator` which returns this object. Would make this clearer, and could be used in the same way this is used below too.

A constant value for the empty iterator itself might be fine too.

Code after:
  constructor(asset: Asset) {
    let existing = valueToSymbols.get(asset);
    if (existing != null) {
      return existing;
    }

    this.#value = asset;
    valueToSymbols.set(asset, this);
  }

  get(exportSymbol: Symbol): ?{|local: Symbol, loc: ?SourceLocation|} {
    return this.#value.symbols?.get(exportSymbol);
  }

  hasExportSymbol(exportSymbol: Symbol): boolean {
    return Boolean(this.#value.symbols?.has(exportSymbol));
  }

  hasLocalSymbol(local: Symbol): boolean {
    if (this.#value.symbols) {
      for (let s of this.#value.symbols.values()) {
        if (local === s.local) return true;
      }
    }
    return false;
  }

  // $FlowFixMe
  [Symbol.iterator]() {
    return this.#value.symbols
      ? this.#value.symbols[Symbol.iterator]()
      : EMPTY_ITERATOR;
  }

  get isCleared() {
    return this.#value.symbols == null;
  }
}

let valueToMutableSymbols: WeakMap<
  Asset | Dependency,
  MutableSymbols,
> = new WeakMap();

class MutableSymbols {
  /*::
  @@iterator(): Iterator<[Symbol, {|local: Symbol, loc: ?SourceLocation|}]> { return ({}: any); }
  */
  #value; // Asset

  constructor(asset: Asset | Dependency) {
Repository: parcel-bundler/parcel
File: packages/core/core/src/public/Config.js (JavaScript)

Code before:
      parse?: boolean,
      exclude?: boolean,
    |},
  ): Promise<ConfigResult | null> {
    let packageKey = options && options.packageKey;
    if (packageKey != null) {
      let pkg = await this.getPackage();
      if (pkg && pkg[packageKey]) {
        return pkg[packageKey];
      }
    }

    let parse = options && options.parse;
    let conf = await loadConfig(
      this.#options.inputFS,
      searchPath,
      filePaths,
      parse == null ? null : {parse},
    );
    if (conf == null) {
      return null;
    }

    if (!options || !options.exclude) {
      if (
        this.#config.resolvedPath == null ||
        this.#config.resolvedPath.endsWith('package.json')
      ) {
        this.setResolvedPath(conf.files[0].filePath);
      } else {
        this.addIncludedFile(conf.files[0].filePath);
      }
    }

    return conf.config;
  }

  getConfig(
    filePaths: Array<FilePath>,
    options: ?{|
      packageKey?: string,
      parse?: boolean,
      exclude?: boolean,
    |},
  ): Promise<ConfigResult | null> {
    return this.getConfigFrom(this.searchPath, filePaths, options);
  }

  async getPackage(): Promise<PackageJSON | null> {
    if (this.#config.pkg) {
      return this.#config.pkg;

Review comment (suggestion):
I'm kind of confused as to how this works, how do you know the first request to `getConfig` is the actual config and not a package.json or something as it was before?

cc @padmaia 

Code after:
    let packageKey = options && options.packageKey;
    if (packageKey != null) {
      let pkg = await this.getPackage();
      if (pkg && pkg[packageKey]) {
        return {
          contents: pkg[packageKey],
          // This should be fine as pkgFilePath should be defined by getPackage()
          filePath: this.#config.pkgFilePath || '',
        };
      }
    }

    let parse = options && options.parse;
    let conf = await loadConfig(
      this.#options.inputFS,
      searchPath,
      filePaths,
      parse == null ? null : {parse},
    );
    if (conf == null) {
      return null;
    }

    let configFilePath = conf.files[0].filePath;
    if (!options || !options.exclude) {
      this.addIncludedFile(configFilePath);
    }

    return {
      contents: conf.config,
      filePath: configFilePath,
    };
  }

  getConfig(
    filePaths: Array<FilePath>,
    options: ?{|
      packageKey?: string,
      parse?: boolean,
      exclude?: boolean,
    |},
  ): Promise<ConfigResultWithFilePath | null> {
    return this.getConfigFrom(this.searchPath, filePaths, options);
  }

  async getPackage(): Promise<PackageJSON | null> {
    if (this.#config.pkg) {
      return this.#config.pkg;
    }

    let pkgConfig = await this.getConfig(['package.json']);
Repository: parcel-bundler/parcel
File: packages/utils/ts-utils/src/loadTSConfig.js (JavaScript)

Code before:
// @flow
import type {Config, PluginOptions} from '@parcel/types';
import typeof TypeScriptModule from 'typescript'; // eslint-disable-line import/no-extraneous-dependencies
import {ParseConfigHost} from './ParseConfigHost';
import path from 'path';
import nullthrows from 'nullthrows';

export async function loadTSConfig(config: Config, options: PluginOptions) {
  let configResult = await config.getConfig(['tsconfig.json']);
  if (!configResult) {
    return;
  }

  let ts: TypeScriptModule = await options.packageManager.require(
    'typescript',
    config.searchPath,
    {autoinstall: options.autoinstall},
  );

  let host = new ParseConfigHost(options.inputFS, ts);
  let parsedConfig = ts.parseJsonConfigFileContent(
    configResult,
    host,
    nullthrows(config.rootDir),
  );

  // Add all of the extended config files to be watched
  for (let file of host.filesRead) {
    config.addIncludedFile(path.resolve(file));
  }

  config.setResult(parsedConfig.options);
}


Review comment (bug):
Note to self: this is failing tests as this should be the tsconfig file path.

Should `config.getConfig` return something like `{content: any, filePath: string}` or something?

Code after:
// @flow
import type {Config, PluginOptions} from '@parcel/types';
import typeof TypeScriptModule from 'typescript'; // eslint-disable-line import/no-extraneous-dependencies
import {ParseConfigHost} from './ParseConfigHost';
import path from 'path';
import nullthrows from 'nullthrows';

export async function loadTSConfig(config: Config, options: PluginOptions) {
  let configResult = await config.getConfig(['tsconfig.json']);
  if (!configResult) {
    return;
  }

  let ts: TypeScriptModule = await options.packageManager.require(
    'typescript',
    config.searchPath,
    {autoinstall: options.autoinstall},
  );

  let host = new ParseConfigHost(options.inputFS, ts);
  let parsedConfig = ts.parseJsonConfigFileContent(
    configResult.contents,
    host,
    path.dirname(nullthrows(configResult.filePath)),
  );

  // Add all of the extended config files to be watched
  for (let file of host.filesRead) {
    config.addIncludedFile(path.resolve(file));
  }

  config.setResult(parsedConfig.options);
}
Repository: parcel-bundler/parcel
File: packages/core/core/src/public/Config.js (JavaScript)

Code before:
    this.#config = config;
    this.#options = options;
    internalConfigToConfig.get(options).set(config, this);
  }

  get env() {
    return new Environment(this.#config.env);
  }

  get searchPath() {
    return this.#config.searchPath;
  }

  get result() {
    return this.#config.result;
  }

  get isSource() {
    return this.#config.isSource;
  }

  get includedFiles() {
    return this.#config.includedFiles;
  }

  get rootDir() {
    if (this.#config.rootDir != null) {
      return this.#config.rootDir;
    }

    for (let filePath of this.includedFiles) {
      if (ROOT_FILES.has(Path.basename(filePath))) {
        this.#config.rootDir = Path.dirname(filePath);
      }
    }

    return this.#config.rootDir || this.#options.rootDir;
  }

  // $FlowFixMe
  setResult(result: any) {
    this.#config.result = result;
  }

  setResultHash(resultHash: string) {
    this.#config.resultHash = resultHash;
  }

  addIncludedFile(filePath: FilePath) {
    if (
      this.#config.rootDir != null &&

Review comment (suggestion):
Should this actually exist?

I also don't think it's calculated correctly at the moment, it should probably take the lock or pkg.json file that is closest to `/` to have a real root? The only real purpose this has is for `resolveFrom`

Code after:

  get searchPath() {
    return this.#config.searchPath;
  }

  get result() {
    return this.#config.result;
  }

  get isSource() {
    return this.#config.isSource;
  }

  get includedFiles() {
    return this.#config.includedFiles;
  }

  // $FlowFixMe
  setResult(result: any) {
    this.#config.result = result;
  }

  setResultHash(resultHash: string) {
    this.#config.resultHash = resultHash;
  }

  addIncludedFile(filePath: FilePath) {
    this.#config.includedFiles.add(filePath);
  }

  addDevDependency(name: PackageName, version?: string) {
    this.#config.devDeps.set(name, version);
  }

  setWatchGlob(glob: Glob) {
    this.#config.watchGlob = glob;
  }

  shouldRehydrate() {
    this.#config.shouldRehydrate = true;
  }

  shouldReload() {
    this.#config.shouldReload = true;
  }

  shouldInvalidateOnStartup() {
    this.#config.shouldInvalidateOnStartup = true;
  }

  async getConfigFrom(
Repository: parcel-bundler/parcel
File: packages/core/utils/src/generateBuildMetrics.js (JavaScript)

Code before:
        currMapping.generated.line === currLine &&
        currMapping.generated.column <= currColumn
      ) {
        sourceSizes[currentSource] += charSize;
      } else {
        unknownOrigin += charSize;
      }

      if (character === '\n') {
        currColumn = 0;
        currLine++;
      } else {
        currColumn++;
      }
    }

    let sizeMap = new Map();
    for (let source of sources) {
      sizeMap.set(source, 0);
    }

    for (let i = 0; i < sourceSizes.length; i++) {
      sizeMap.set(sources[i], sourceSizes[i]);
    }

    sizeMap.set('unknown-origin', unknownOrigin);

    return sizeMap;
  }
}

async function createBundleStats(
  bundle: NamedBundle,
  fs: FileSystem,
  projectRoot: FilePath,
) {
  let filePath = bundle.filePath;
  let sourcemapSizes = await getSourcemapSizes(filePath, fs, projectRoot);

  let assets: Map<string, AssetStats> = new Map();
  bundle.traverseAssets(asset => {
    let filePath = path.normalize(asset.filePath);
    assets.set(filePath, {
      filePath,
      size: asset.stats.size,
      originalSize: asset.stats.size,
      time: asset.stats.time,
    });
  });

  let assetsReport: Array<AssetStats> = [];

Review comment (suggestion):
Maybe do `sizeMap.set(null, unknownOrigin)` instead so that the reporter can do some special formatting for this pseudo asset? Right now it looks like a regular asset that is just called "unknown-origin".

Code after:
        currMapping.generated.line === currLine &&
        currMapping.generated.column <= currColumn
      ) {
        sourceSizes[currentSource] += charSize;
      } else {
        unknownOrigin += charSize;
      }

      if (character === '\n') {
        currColumn = 0;
        currLine++;
      } else {
        currColumn++;
      }
    }

    let sizeMap = new Map();
    for (let i = 0; i < sourceSizes.length; i++) {
      sizeMap.set(sources[i], sourceSizes[i]);
    }

    sizeMap.set('', unknownOrigin);

    return sizeMap;
  }
}

async function createBundleStats(
  bundle: NamedBundle,
  fs: FileSystem,
  projectRoot: FilePath,
) {
  let filePath = bundle.filePath;
  let sourcemapSizes = await getSourcemapSizes(filePath, fs, projectRoot);

  let assets: Map<string, AssetStats> = new Map();
  bundle.traverseAssets(asset => {
    let filePath = path.normalize(asset.filePath);
    assets.set(filePath, {
      filePath,
      size: asset.stats.size,
      originalSize: asset.stats.size,
      time: asset.stats.time,
    });
  });

  let assetsReport: Array<AssetStats> = [];
  if (sourcemapSizes && sourcemapSizes.size) {
    assetsReport = Array.from(sourcemapSizes.keys()).map((filePath: string) => {
      let foundSize = sourcemapSizes.get(filePath) || 0;
      let stats = assets.get(filePath) || {