ronantakizawa/codereview-bench huggingface dataset:
Repository: parcel-bundler/parcel
File: packages/packagers/html/src/HTMLPackager.js (JavaScript)

Code before:
        .getBundlesInBundleGroup(bundleGroup)
        .filter(
          bundle =>
            !bundle
              .getEntryAssets()
              .some(asset => asset.id === bundleGroup.entryAssetId)
        );
      return p.concat(bundles);
    }, []);

    let {html} = await posthtml([
      insertBundleReferences.bind(this, bundles),
      replaceInlineAssetContent.bind(this, bundleGraph, getBundleResult)
    ]).process(code);

    return {contents: html};
  }
});

async function getAssetContent(
  bundleGraph: BundleGraph,
  getBundleResult,
  assetId
): Promise<?Blob> {
  let inlineBundle: ?Bundle;
  bundleGraph.traverseBundles((bundle, context, {stop}) => {
    let mainAsset = bundle.getMainEntry();
    if (mainAsset && mainAsset.meta && mainAsset.meta.inlineKey === assetId) {
      inlineBundle = bundle;
      stop();
    }
  });

  if (inlineBundle) {
    const bundleResult = await getBundleResult(inlineBundle, bundleGraph);

    return bundleResult.contents;
  }

  return null;
}

async function replaceInlineAssetContent(
  bundleGraph: BundleGraph,
  getBundleResult,
  tree
) {
  const inlineNodes = [];
  tree.walk(node => {
    if (node.attrs && node.attrs['data-parcel-key']) {
      inlineNodes.push(node);

Review comment (performance):
This could probably be optimized to traverse the graph once at the start of packaging, and make a map of inline asset keys to bundles. This way you'd avoid re-traversing the graph for each one. Probably not a big deal, but might be nice as a future improvement.

Code after:
        .getBundlesInBundleGroup(bundleGroup)
        .filter(
          bundle =>
            !bundle
              .getEntryAssets()
              .some(asset => asset.id === bundleGroup.entryAssetId)
        );
      return p.concat(bundles);
    }, []);

    let {html} = await posthtml([
      insertBundleReferences.bind(this, bundles),
      replaceInlineAssetContent.bind(this, bundleGraph, getInlineBundleContents)
    ]).process(code);

    return {contents: html};
  }
});

async function getAssetContent(
  bundleGraph: BundleGraph,
  getInlineBundleContents,
  assetId
): Promise<?Blob> {
  let inlineBundle: ?Bundle;
  bundleGraph.traverseBundles((bundle, context, {stop}) => {
    let mainAsset = bundle.getMainEntry();
    if (mainAsset && mainAsset.uniqueKey === assetId) {
      inlineBundle = bundle;
      stop();
    }
  });

  if (inlineBundle) {
    const bundleResult = await getInlineBundleContents(
      inlineBundle,
      bundleGraph
    );

    return bundleResult.contents;
  }

  return null;
}

async function replaceInlineAssetContent(
  bundleGraph: BundleGraph,
  getInlineBundleContents,
  tree
) {
  const inlineNodes = [];
Repository: parcel-bundler/parcel
File: packages/core/codeframe/src/codeframe.js (JavaScript)

Code before:
|};

const NEWLINE = /\r\n|[\n\r\u2028\u2029]/;

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>,
  // $FlowFixMe
  inputOpts: CodeFrameOptionsInput = {}
): string {
  if (highlights.length < 1) return '';

  // Replace all tabs with a space to simplify charcount
  code = code.replace(/\t/g, ' ');

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

  const highlighter = (s: string, bold?: boolean) => {
    if (opts.useColor) {
      let redString = chalk.red(s);
      return bold ? chalk.bold(redString) : redString;
    }

    return s;
  };

  const lineNumberPrefixer = (params: {|
    lineNumber?: string,
    endLine: string,
    isHighlighted: boolean

Review comment (bug):
Could you add a comment here that tabs should be treated as a single space
 https://github.com/parcel-bundler/parcel/blob/5e38099af6d4cf53d5d6e411b11bbd706ffc7434/packages/core/diagnostic/src/diagnostic.js#L10-L15

Are you sure this is what babel does (ignore the wrong highlight locations for now)? The indentation is barely noticeable:

![Bildschirmfoto 2019-11-01 um 15 25 41](https://user-images.githubusercontent.com/4586894/68031416-e7d39d80-fcbb-11e9-9477-c886084eda0f.png)


Code after:
|};

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

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>,
  // $FlowFixMe
  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,
    padding: inputOpts.padding || {
      before: 1,
      after: 2
    }
  };

  const highlighter = (s: string, bold?: boolean) => {
    if (opts.useColor) {
      let redString = chalk.red(s);
      return bold ? chalk.bold(redString) : redString;
    }

    return s;
  };

  const lineNumberPrefixer = (params: {|
    lineNumber?: string,
    endLine: string,
    isHighlighted: boolean
  |}) => {
Repository: parcel-bundler/parcel
File: packages/core/core/src/ResolverRunner.js (JavaScript)

Code before:
type Opts = {|
  config: ParcelConfig,
  options: ParcelOptions
|};

export default class ResolverRunner {
  config: ParcelConfig;
  options: ParcelOptions;
  pluginOptions: PluginOptions;

  constructor({config, options}: Opts) {
    this.config = config;
    this.options = options;
    this.pluginOptions = new PluginOptions(this.options);
  }

  async getThrowableDiagnostic(dependency: Dependency, message: string) {
    let diagnostic: Diagnostic = {
      message,
      origin: '@parcel/resolver-default'
    };

    if (dependency.loc && dependency.sourcePath) {
      diagnostic.filePath = dependency.sourcePath;
      diagnostic.codeFrame = {
        code: await this.options.inputFS.readFile(
          dependency.sourcePath,
          'utf8'
        ),
        codeHighlights: dependency.loc
          ? [
              {
                start: dependency.loc.start
                  ? dependency.loc.start
                  : dependency.loc,
                end: dependency.loc.end ? dependency.loc.end : dependency.loc
              }
            ]
          : []
      };
    }

    return new ThrowableDiagnostic({diagnostic});
  }

  async resolve(dependency: Dependency): Promise<?AssetRequestDesc> {
    let dep = new PublicDependency(dependency);
    report({
      type: 'buildProgress',
      phase: 'resolving',
      dependency: dep

Review comment (bug):
How would I go about getting the asset source in the resolver?

Reading it from fileSystem would result in incorrect codeFrames... It has to be a snapshot from the moment the location data was extracted. Which is not possible atm afaik...

Code after:
type Opts = {|
  config: ParcelConfig,
  options: ParcelOptions
|};

export default class ResolverRunner {
  config: ParcelConfig;
  options: ParcelOptions;
  pluginOptions: PluginOptions;

  constructor({config, options}: Opts) {
    this.config = config;
    this.options = options;
    this.pluginOptions = new PluginOptions(this.options);
  }

  async getThrowableDiagnostic(dependency: Dependency, message: string) {
    let diagnostic: Diagnostic = {
      message,
      origin: '@parcel/resolver-default'
    };

    if (dependency.loc && dependency.sourcePath) {
      diagnostic.filePath = dependency.sourcePath;
      diagnostic.codeFrame = {
        code: await this.options.inputFS.readFile(
          dependency.sourcePath,
          'utf8'
        ),
        codeHighlights: dependency.loc
          ? [{start: dependency.loc.start, end: dependency.loc.end}]
          : []
      };
    }

    return new ThrowableDiagnostic({diagnostic});
  }

  async resolve(dependency: Dependency): Promise<?AssetRequestDesc> {
    let dep = new PublicDependency(dependency);
    report({
      type: 'buildProgress',
      phase: 'resolving',
      dependency: dep
    });

    let resolvers = await this.config.getResolvers();

    let pipeline;
    let filePath;
    let validPipelines = new Set(this.config.getNamedPipelines());
Repository: parcel-bundler/parcel
File: packages/transformers/js/src/visitors/dependencies.js (JavaScript)

Code before:
function isPromiseResolve(node) {
  return (
    types.isCallExpression(node) &&
    types.isMemberExpression(node.callee) &&
    types.isIdentifier(node.callee.object) &&
    node.callee.object.name === 'Promise' &&
    node.callee.property.name === 'resolve'
  );
}

function getFunctionParent(ancestors) {
  for (let i = ancestors.length - 1; i >= 0; i--) {
    if (types.isFunction(ancestors[i])) {
      return ancestors[i - 1];
    }
  }
}

function addDependency(
  asset,
  node,
  opts: ?{|isAsync?: boolean, isOptional?: boolean|}
) {
  asset.addDependency({
    moduleSpecifier: node.value,
    loc: node.loc && createDependencyLocation(node.loc.start, node.value, 1, 1),
    isAsync: opts ? opts.isAsync : false,
    isOptional: opts ? opts.isOptional : false
  });
}

function addURLDependency(asset, node, opts = {}) {
  node.value = asset.addURLDependency(node.value, {
    loc: node.loc && createDependencyLocation(node.loc.start, node.value, 1, 1),
    ...opts
  });
  invariant(asset.ast);
  asset.ast.isDirty = true;
}


Review comment (suggestion):
I think these are off by one line. I'm getting this:

<img width="822" alt="image" src="https://user-images.githubusercontent.com/19409/69980679-b8a08e00-14fe-11ea-9300-6ce972ea1a50.png">

Changing `lineOffset` to `0` makes it work...

Code after:
        : types.functionExpression(
            null,
            [],
            types.blockStatement([types.returnStatement(requireClone)])
          );

      // Add the original function as an additional promise chain
      let replacement = types.callExpression(
        types.memberExpression(
          types.clone(functionParent),
          types.identifier('then')
        ),
        [fn]
      );

      morph(functionParent, replacement);
      ast.isDirty = true;
    }

    return true;
  }

  // Promise.resolve(require('foo'))
  if (isPromiseResolve(parent) && parent.arguments[0] === requireNode) {
    return true;
  }

  // new Promise(resolve => resolve(require('foo')))
  // new Promise(resolve => { resolve(require('foo')) })
  // new Promise(function (resolve) { resolve(require('foo')) })
  if (
    functionParent &&
    types.isCallExpression(parent) &&
    types.isIdentifier(parent.callee) &&
    types.isNewExpression(functionParent) &&
    types.isIdentifier(functionParent.callee) &&
    functionParent.callee.name === 'Promise' &&
    types.isFunction(functionParent.arguments[0]) &&
    types.isIdentifier(functionParent.arguments[0].params[0]) &&
    parent.callee.name === functionParent.arguments[0].params[0].name
  ) {
    return true;
  }
}

function isPromiseResolve(node) {
  return (
    types.isCallExpression(node) &&
    types.isMemberExpression(node.callee) &&
    types.isIdentifier(node.callee.object) &&
    node.callee.object.name === 'Promise' &&
Repository: parcel-bundler/parcel
File: packages/transformers/postcss/src/loadConfig.js (JavaScript)

Code before:
    config.searchPath,
    options,
  );

  return config.setResult({
    raw: configFile,
    hydrated: {
      plugins,
      from: config.searchPath,
      to: config.searchPath,
      modules,
    },
  });
}

export async function load({
  config,
  options,
  logger,
}: {|
  config: Config,
  options: PluginOptions,
  logger: PluginLogger,
|}) {
  let configFile: any = await config.getConfig(
    ['.postcssrc', '.postcssrc.json', 'postcss.config.js', '.postcssrc.js'],
    {packageKey: 'postcss'},
  );

  let configPath = config.resolvedPath;
  if (configPath) {
    if (path.extname(configPath) === '.js') {
      logger.warn({
        message:
          'WARNING: Using a JavaScript PostCSS config file means losing out on caching features of Parcel. Use a .postcssrc(.json) file whenever possible.',
      });

      config.shouldInvalidateOnStartup();
    }

    if (configFile) {
      if (typeof configFile !== 'object') {
        throw new Error('PostCSS config should be an object.');
      }

      if (
        configFile.plugins == null ||
        typeof configFile.plugins !== 'object' ||
        Object.keys(configFile.plugins) === 0
      ) {
        throw new Error('PostCSS config must have plugins');

Review comment (bug):
`package.json` key config seems to be broken from the test I wrote, not entirely sure if this is a known issue or if I just use it incorrectly

Code after:
    options,
  );

  return config.setResult({
    raw: configFile,
    hydrated: {
      plugins,
      from: config.searchPath,
      to: config.searchPath,
      modules: modulesConfig,
    },
  });
}

export async function load({
  config,
  options,
  logger,
}: {|
  config: Config,
  options: PluginOptions,
  logger: PluginLogger,
|}) {
  let configFile: any = await config.getConfig(
    ['.postcssrc', '.postcssrc.json', '.postcssrc.js', 'postcss.config.js'],
    {packageKey: 'postcss'},
  );

  let configPath = config.resolvedPath;
  if (configPath) {
    if (path.extname(configPath) === '.js') {
      logger.warn({
        message:
          'WARNING: Using a JavaScript PostCSS config file means losing out on caching features of Parcel. Use a .postcssrc(.json) file whenever possible.',
      });

      config.shouldInvalidateOnStartup();
    }

    if (configFile) {
      if (typeof configFile !== 'object') {
        throw new Error('PostCSS config should be an object.');
      }

      if (
        configFile.plugins == null ||
        typeof configFile.plugins !== 'object' ||
        Object.keys(configFile.plugins) === 0
      ) {
        throw new Error('PostCSS config must have plugins');
      }
Repository: parcel-bundler/parcel
File: packages/transformers/postcss/src/loadConfig.js (JavaScript)

Code before:
        }

        if (typeof p === 'string') {
          if (p.startsWith('.')) {
            logger.warn({
              message:
                'WARNING: Using relative PostCSS plugins means losing out on caching features of Parcel. Bundle this plugin up in a package or use a monorepo to resolve this issue.',
            });

            config.shouldInvalidateOnStartup();
          }

          config.addDevDependency(p);
        }
      }
    }
  }

  return configHydrator(configFile, config, options);
}

export function preSerialize(config: Config) {
  if (!config.result) return;

  if (config.result.raw.__contains_functions) {
    // $FlowFixMe
    config.result = {};
  } else {
    // $FlowFixMe
    config.result = {
      raw: config.result.raw,
    };
  }
}

export function postDeserialize(config: Config, options: PluginOptions) {
  return configHydrator(config.result.raw, config, options);
}


Review comment (suggestion):
I think the flow errors you are suppressing here are actually useful. The variable `config` here is an instance of a public class, which has an underlying config that is not actually being mutated here. 

Code after:

        if (typeof p === 'string') {
          if (p.startsWith('.')) {
            logger.warn({
              message:
                'WARNING: Using relative PostCSS plugins means losing out on caching features of Parcel. Bundle this plugin up in a package or use a monorepo to resolve this issue.',
            });

            config.shouldInvalidateOnStartup();
          }

          config.addDevDependency(p);
        }
      }
    }
  }

  return configHydrator(configFile, config, options);
}

export function preSerialize(config: Config) {
  if (!config.result) return;

  // Ensure we dont pass functions to the serialiser
  if (config.result.raw.__contains_functions) {
    config.result.raw = {};
  }

  // This gets re-hydrated in Deserialize, so never store this.
  // It also usually contains a bunch of functions so bad idea anyway...
  config.result.hydrated = {};
}

export function postDeserialize(config: Config, options: PluginOptions) {
  return configHydrator(config.result.raw, config, options);
}
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 svgToJsx from 'svg-to-jsx';

import * as babelParser from '@babel/parser';

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

  return `Svg${pascalCaseFileName}`;
}

export default new Transformer({
  async parse({asset}) {
    let code = await asset.getCode();
    let componentName = getComponentName(asset.filePath);
    let jsx = await svgToJsx(code);

    code = `import React from 'react';
  export default function ${componentName}(props) {
    return ${jsx.replace(/<svg (.*)>/, '<svg $1 {...props}>')};
  }
  `;

    asset.setCode(code);

    return {
      type: 'babel',
      version: '7.0.0',
      isDirty: false,
      program: babelParser.parse(code, {
        filename: asset.filePath.replace('.svg', '.js'),
        strictMode: false,
        sourceType: 'module',
        plugins: ['exportDefaultFrom', 'jsx'],
      }),
    };
  },

Review comment (suggestion):
If you move the logic from parse to transform you don't have to return an ast and can just use asset.setcode and return the asset wihtout needing babel at all.

That's what we've done in a couple transformers that dont' utilise ast's

Code after:
// @flow

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

import path from 'path';
import camelcase from 'camelcase';
import svgToJsx from 'svg-to-jsx';

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

  return `Svg${pascalCaseFileName}`;
}

export default new Transformer({
  async transform({asset}) {
    let code = await asset.getCode();
    let componentName = getComponentName(asset.filePath);
    let jsx = await svgToJsx(code);

    code = `import React from 'react';
    export default function ${componentName}(props) {
      return ${jsx.replace(/<svg (.*)>/, '<svg $1 {...props}>')};
    }
    `;

    asset.type = 'js';
    asset.setCode(code);

    return [asset];
  },
});
Repository: parcel-bundler/parcel
File: packages/transformers/svgo/src/SVGOTransformer.js (JavaScript)

Code before:
import SVGO from 'svgo';

const defaultConfig = {
  plugins: [{prefixIds: true}],
};

export default new Transformer({
  async getConfig({asset}) {
    let svgoConfig = await asset.getConfig(
      [
        '.svgorc',
        '.svgorc.json',
        '.svgorc.yaml',
        '.svgorc.yml',
        'svgo.config.js',
        '.svgo.yml',
      ],
      {
        packageKey: 'svgo',
      },
    );

    svgoConfig = svgoConfig || {};
    svgoConfig = {...defaultConfig, ...svgoConfig};

    this.svgo = new SVGO(svgoConfig);

    return svgoConfig;
  },

  transform({asset}) {
    return [asset];
  },

  async generate({asset}) {
    let code = await asset.getCode();
    let res = await this.svgo.optimize(code);

    return {code: res.data};
  },
});


Review comment (suggestion):
Not entirely sure if this is a good idea as it's not guaranteed that this state will be kept in other functions. It currently works but we could shift around a bit in transformrunner and break this.

Probably best to initialise svgo where you actually use it and keep the config to the purest JSON form possible. This will also help us improve this plugin for better caching in the future :)

Code after:
import SVGO from 'svgo';

const defaultConfig = {
  plugins: [{prefixIds: true}],
};

export default new Transformer({
  async getConfig({asset}) {
    let config = await asset.getConfig(
      [
        '.svgorc',
        '.svgorc.json',
        '.svgorc.yaml',
        '.svgorc.yml',
        'svgo.config.js',
        '.svgo.yml',
      ],
      {
        packageKey: 'svgo',
      },
    );

    config = {...defaultConfig, ...config};

    return config;
  },

  async transform({asset, config}) {
    let code = await asset.getCode();
    let svgo = new SVGO(config);
    let res = await svgo.optimize(code);

    asset.setCode(res.data);

    return [asset];
  },
});
Repository: parcel-bundler/parcel
File: packages/transformers/elm/src/ElmTransformer.js (JavaScript)

Code before:
import {Transformer} from '@parcel/plugin';
import commandExists from 'command-exists';
import path from 'path';
import {minify} from 'terser';
import ThrowableDiagnostic from '@parcel/diagnostic';

export default new Transformer({
  async transform({asset, options}) {
    const elm = await setupElmCompiler(asset, options);

    const config = {
      cwd: path.dirname(asset.filePath),
      debug: !options.env.PARCEL_ELM_NO_DEBUG && options.mode !== 'production',
      optimize: asset.env.minify,
    };
    asset.invalidateOnEnvChange('PARCEL_ELM_NO_DEBUG');
    ResolveDependenciesFromAsset(elm, asset);

    let code = await compileToString(elm, asset, config);
    if (options.hot) {
      code = await injectHotModuleReloadRuntime(code, asset.filePath, options);
    }
    if (config.optimize) code = await minifyElmOutput(code);

    asset.type = 'js';
    asset.setCode(code);
    return [asset];
  },
});

async function setupElmCompiler(asset, options) {
  ensureElmIsInstalled();
  await ensureElmJson(asset);
  return options.packageManager.require('node-elm-compiler', asset.filePath, {

Review comment (refactor):
Can you inline this call to `setupElmCompiler `, calling a helper function that also calls even more helper functions becomes hard to read...

Code after:
// @flow strict-local

import {Transformer} from '@parcel/plugin';
import commandExists from 'command-exists';
import spawn from 'cross-spawn';
import path from 'path';
import {minify} from 'terser';
import nullthrows from 'nullthrows';
import ThrowableDiagnostic from '@parcel/diagnostic';

let isWorker;
try {
  let worker_threads = require('worker_threads');
  isWorker = worker_threads.threadId > 0;
} catch (_) {
  isWorker = false;
}

export default (new Transformer({
  async loadConfig({config, options}) {
    const elmConfig = await config.getConfig(['elm.json']);
    if (!elmConfig) {
      await elmBinaryPath(config.searchPath, options); // Check if elm is even installed
      throw new ThrowableDiagnostic({
        diagnostic: {
          message: "The 'elm.json' file is missing.",
          hints: [
            "Initialize your elm project by running 'elm init'",
            "If you installed elm as project dependency then run 'yarn elm init' or 'npx elm init'",
          ],
        },
      });
    }
    config.setResult(elmConfig.contents);