ronantakizawa/codereview-bench huggingface dataset:
Repository: parcel-bundler/parcel
File: packages/optimizers/svgo/src/SVGOOptimizer.js (JavaScript)

Code before:
// @flow

import {Optimizer} from '@parcel/plugin';
import ThrowableDiagnostic from '@parcel/diagnostic';
import {blobToString} from '@parcel/utils';

import * as svgo from 'svgo';
import path from 'path';

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

    if (configFile) {
      let isJavascript = path.extname(configFile.filePath) === '.js';
      if (isJavascript) {
        config.invalidateOnStartup();
      }
      return configFile.contents;
    }
  },

  async optimize({bundle, contents, config}) {
    if (!bundle.env.shouldOptimize) {
      return {contents};
    }

    let code = await blobToString(contents);
    let result = svgo.optimize(code, config);
    if (result.error != null) {
      throw new ThrowableDiagnostic({
        diagnostic: {

Review comment (suggestion):
The only config files read by svgo 2 itself is `svgo.config.js`, so maybe we should only support `svgo.config.js(on)`

Code after:
// @flow

import {Optimizer} from '@parcel/plugin';
import ThrowableDiagnostic from '@parcel/diagnostic';
import {blobToString} from '@parcel/utils';

import * as svgo from 'svgo';
import path from 'path';

export default (new Optimizer({
  async loadConfig({config}) {
    let configFile = await config.getConfig([
      'svgo.config.js',
      'svgo.config.json',
    ]);

    if (configFile) {
      let isJavascript = path.extname(configFile.filePath) === '.js';
      if (isJavascript) {
        config.invalidateOnStartup();
      }
      return configFile.contents;
    }
  },

  async optimize({bundle, contents, config}) {
    if (!bundle.env.shouldOptimize) {
      return {contents};
    }

    let code = await blobToString(contents);
    let result = svgo.optimize(code, config);
    if (result.error != null) {
      throw new ThrowableDiagnostic({
        diagnostic: {
          message: result.error,
        },
      });
    }

    return {contents: result.data};
  },
}): Optimizer);
Repository: parcel-bundler/parcel
File: packages/transformers/svg/src/SVGTransformer.js (JavaScript)

Code before:

import {Transformer} from '@parcel/plugin';
import nullthrows from 'nullthrows';
import semver from 'semver';
import collectDependencies from './dependencies';
import parse from 'posthtml-parser';
import render from 'posthtml-render';

export default (new Transformer({
  canReuseAST({ast}) {
    return ast.type === 'posthtml' && semver.satisfies(ast.version, '^0.4.0');
  },

  async parse({asset}) {
    return {
      type: 'posthtml',
      version: '0.4.1',
      program: parse(await asset.getCode(), {
        directives: [
          {
            name: /^\?/,
            start: '<',
            end: '>',
          },
        ],
        lowerCaseAttributeNames: true,
        sourceLocations: true,
        xmlMode: true,
      }),
    };
  },

  async transform({asset}) {
    const ast = nullthrows(await asset.getAST());

    collectDependencies(asset, ast);

    return [asset];
  },

  generate({ast}) {
    return {
      content: render(ast.program),
    };
  },
}): Transformer);


Review comment (security):
I think this should be false? SVG attributes are case sensitive. They are only case insensitive when embedded in an HTML file, but that should already be handled by the HTML transformer.

Code after:

import {Transformer} from '@parcel/plugin';
import nullthrows from 'nullthrows';
import semver from 'semver';
import collectDependencies from './dependencies';
import parse from 'posthtml-parser';
import render from 'posthtml-render';

export default (new Transformer({
  canReuseAST({ast}) {
    return ast.type === 'posthtml' && semver.satisfies(ast.version, '^0.4.0');
  },

  async parse({asset}) {
    return {
      type: 'posthtml',
      version: '0.4.1',
      program: parse(await asset.getCode(), {
        directives: [
          {
            name: /^\?/,
            start: '<',
            end: '>',
          },
        ],
        sourceLocations: true,
        xmlMode: true,
      }),
    };
  },

  async transform({asset}) {
    const ast = nullthrows(await asset.getAST());

    collectDependencies(asset, ast);

    return [asset];
  },

  generate({ast}) {
    return {
      content: render(ast.program),
    };
  },
}): Transformer);
Repository: parcel-bundler/parcel
File: packages/transformers/svg-react/src/SvgReactTransformer.js (JavaScript)

Code before:
  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 transform({asset}) {
    let code = await asset.getCode();
    let componentName = getComponentName(asset.filePath);

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

    asset.type = 'jsx';
    asset.bundleBehavior = 'inline';
    asset.setCode(jsx);

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


Review comment (suggestion):
I think this should be `null` instead. `inline` means it will create a separate bundle, and then inline it, but we really just want it to be in the same bundle as the parent javascript.

```suggestion
    asset.bundleBehavior = null;
```

Code after:
  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 transform({asset}) {
    let code = await asset.getCode();
    let componentName = getComponentName(asset.filePath);

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

    asset.type = 'jsx';
    asset.bundleBehavior = null;
    asset.setCode(jsx);

    return [asset];
  },
}): Transformer);
Repository: parcel-bundler/parcel
File: packages/runtimes/service-worker/src/ServiceWorkerRuntime.js (JavaScript)

Code before:

    let hasDep;
    bundle.traverse((node, _, actions) => {
      if (
        node.type === 'dependency' &&
        node.value.specifier === '@parcel/service-worker'
      ) {
        hasDep = true;
        actions.stop();
      }
    });

    if (!hasDep) {
      return [];
    }

    let manifest = [];
    bundleGraph.traverseBundles(b => {
      if (b.bundleBehavior === 'inline' || b.id === bundle.id) {
        return;
      }

      manifest.push(urlJoin(b.target.publicUrl, b.name));
    });

    let code = `import {_register} from '@parcel/service-worker';
const manifest = ${JSON.stringify(manifest)};
const version = ${JSON.stringify(bundle.hashReference)};
_register(manifest, version);
`;

    return [
      {
        filePath: __filename,
        code,
        isEntry: true,
        env: {sourceType: 'module'},
      },
    ];
  },
}): Runtime);


Review comment (suggestion):
So this imports `@parcel/service-worker` relative to the runtime? It should rather be resolved from the user's package. Yarn 2 would still break

Code after:

    let asset;
    bundle.traverse((node, _, actions) => {
      if (
        node.type === 'dependency' &&
        node.value.specifier === '@parcel/service-worker'
      ) {
        asset = bundleGraph.getResolvedAsset(node.value, bundle);
        actions.stop();
      }
    });

    if (!asset) {
      return [];
    }

    let manifest = [];
    bundleGraph.traverseBundles(b => {
      if (b.bundleBehavior === 'inline' || b.id === bundle.id) {
        return;
      }

      manifest.push(urlJoin(b.target.publicUrl, b.name));
    });

    let code = `import {_register} from '@parcel/service-worker';
const manifest = ${JSON.stringify(manifest)};
const version = ${JSON.stringify(bundle.hashReference)};
_register(manifest, version);
`;

    return [
      {
        filePath: asset.filePath,
        code,
        isEntry: true,
        env: {sourceType: 'module'},
      },
    ];
  },
}): Runtime);
Repository: parcel-bundler/parcel
File: packages/transformers/image/src/ImageTransformer.js (JavaScript)

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

// from https://github.com/lovell/sharp/blob/df7b8ba73808fc494be413e88cfb621b6279218c/lib/output.js#L6-L17
const FORMATS = new Map([
  ['heic', 'heif'],
  ['heif', 'heif'],
  ['avif', 'avif'],
  ['jpeg', 'jpeg'],
  ['jpg', 'jpeg'],
  ['png', 'png'],
  ['raw', 'raw'],
  ['tiff', 'tiff'],
  ['webp', 'webp'],
  ['gif', 'gif'],
]);

const SHARP_RANGE = '^0.28.3';

export default (new Transformer({
  async transform({asset, options}) {
    asset.bundleBehavior = 'isolated';

    let width =
      typeof asset.query.width === 'string'
        ? parseInt(asset.query.width, 10)
        : null;
    let height =
      typeof asset.query.height === 'string'
        ? parseInt(asset.query.height, 10)
        : null;
    let quality =
      typeof asset.query.quality === 'string'
        ? parseInt(asset.query.quality, 10)
        : undefined;
    let format =
      typeof asset.query.as === 'string'
        ? asset.query.as.toLowerCase().trim()
        : null;

    if (width || height || quality || format) {
      let inputBuffer = await asset.getBuffer();
      let sharp = await options.packageManager.require(
        'sharp',
        asset.filePath,
        {
          range: SHARP_RANGE,
          shouldAutoInstall: options.shouldAutoInstall,
        },
      );

      let imagePipeline = sharp(inputBuffer);

Review comment (refactor):
ugh that's annoying... should we even support duplicate query params?

Or maybe `asset.query` should be a `URLSearchParams` object?

Code after:
import {validateConfig} from './validateConfig';
import {Transformer} from '@parcel/plugin';
import nullthrows from 'nullthrows';
import WorkerFarm from '@parcel/workers';
import loadSharp from './loadSharp';

// from https://github.com/lovell/sharp/blob/df7b8ba73808fc494be413e88cfb621b6279218c/lib/output.js#L6-L17
const FORMATS = new Map([
  ['jpeg', 'jpeg'],
  ['jpg', 'jpeg'],
  ['png', 'png'],
  ['webp', 'webp'],
  ['gif', 'gif'],
  ['tiff', 'tiff'],
  ['avif', 'avif'],
  ['heic', 'heif'],
  ['heif', 'heif'],
]);

let isSharpLoadedOnMainThread = false;

export default (new Transformer({
  async loadConfig({config}) {
    let configFile: any = await config.getConfig(
      ['sharp.config.json'], // '.sharprc', '.sharprc.json'
      {packageKey: 'sharp'},
    );

    if (configFile?.contents) {
      validateConfig(configFile.contents, configFile.filePath);
      return configFile.contents;
    } else {
      return {};
    }
  },

  async transform({config, asset, options}) {
    asset.bundleBehavior = 'isolated';

    const originalFormat = FORMATS.get(asset.type);
    if (!originalFormat) {
      throw new Error(
        `The image transformer does not support ${asset.type} images.`,
      );
    }

    const width = asset.query.has('width')
      ? parseInt(asset.query.get('width'), 10)
      : null;
    const height = asset.query.has('height')
      ? parseInt(asset.query.get('height'), 10)
Repository: parcel-bundler/parcel
File: packages/transformers/css/src/CSSTransformer.js (JavaScript)

Code before:
      // }
      isDirty = true;
    });

    program.walkDecls(decl => {
      if (URL_RE.test(decl.value)) {
        let parsed = valueParser(decl.value);
        let isDeclDirty = false;

        parsed.walk(node => {
          if (
            node.type === 'function' &&
            node.value === 'url' &&
            node.nodes.length > 0 &&
            !node.nodes[0].value.startsWith('#') // IE's `behavior: url(#default#VML)`
          ) {
            let url = asset.addURLDependency(node.nodes[0].value, {
              loc:
                decl.source && decl.source.start
                  ? createLoc(
                      decl.source.start,
                      node.nodes[0].value,
                      0,
                      node.nodes[0].sourceIndex,
                    )
                  : createLoc({line: 1, column: 1}, ''),
            });
            isDeclDirty = node.nodes[0].value !== url;
            node.nodes[0].value = url;
          }
        });

        if (isDeclDirty) {
          decl.value = parsed.toString();
          isDirty = true;
        }
      }
    });

    if (isDirty) {
      asset.setAST({
        ...ast,
        program: program.toJSON(),
      });
    }

    return [asset];
  },

  async generate({asset, ast, options}) {
    let result = await postcss().process(postcss.fromJSON(ast.program), {

Review comment (suggestion):
I think `loc` should rather be `undefined` if we can't determine the position. 

Code after:
      // }
      isDirty = true;
    });

    program.walkDecls(decl => {
      if (URL_RE.test(decl.value)) {
        let parsed = valueParser(decl.value);
        let isDeclDirty = false;

        parsed.walk(node => {
          if (
            node.type === 'function' &&
            node.value === 'url' &&
            node.nodes.length > 0 &&
            !node.nodes[0].value.startsWith('#') // IE's `behavior: url(#default#VML)`
          ) {
            let url = asset.addURLDependency(node.nodes[0].value, {
              loc:
                decl.source &&
                decl.source.start &&
                createLoc(
                  decl.source.start,
                  node.nodes[0].value,
                  0,
                  node.nodes[0].sourceIndex,
                ),
            });
            isDeclDirty = node.nodes[0].value !== url;
            node.nodes[0].value = url;
          }
        });

        if (isDeclDirty) {
          decl.value = parsed.toString();
          isDirty = true;
        }
      }
    });

    if (isDirty) {
      asset.setAST({
        ...ast,
        program: program.toJSON(),
      });
    }

    return [asset];
  },

  async generate({asset, ast, options}) {
    let result = await postcss().process(postcss.fromJSON(ast.program), {
Repository: parcel-bundler/parcel
File: packages/optimizers/image/src/ImageOptimizer.js (JavaScript)

Code before:
// @flow
import {Optimizer} from '@parcel/plugin';
import {blobToBuffer, relativePath} from '@parcel/utils';
import {optimize} from '../native';

export default (new Optimizer({
  async optimize({bundle, contents, logger, options}) {
    if (!bundle.env.shouldOptimize) {
      return {contents};
    }

    let buffer = await blobToBuffer(contents);

    // Attempt to optimize it, if the optimize fails we log a warning...
    try {
      let optimized = optimize(bundle.type, buffer);
      return {
        contents: optimized.length < buffer.length ? optimized : buffer,
      };
    } catch (err) {
      const filepath = bundle.getMainEntry()?.filePath;
      const filename = filepath
        ? relativePath(options.projectRoot, filepath)
        : 'unknown';
      logger.warn({
        message: `Could not optimize image ${filename}: ${err.message}`,
        stack: err.stack,
      });
    }

    return {contents: buffer};
  },
}): Optimizer);


Review comment (security):
1.  Use `md` to escape characters in the filepath: https://github.com/parcel-bundler/parcel/blob/c444af0d739438be7c73e01b0f2a6b4ff10a0903/packages/namers/default/src/DefaultNamer.js#L58
2. I think this should use the regular `path.relative`, so that Windows users see the correct path separator
3. Sometimes we use cwd instead of the project root 🤷  https://github.com/parcel-bundler/parcel/blob/7d4d53a098755d3dd494eb28d593f1bb4d5c1ec7/packages/transformers/babel/src/config.js#L323-L332

Code after:
// @flow
import path from 'path';
import process from 'process';
import {Optimizer} from '@parcel/plugin';
import {blobToBuffer} from '@parcel/utils';
import {md} from '@parcel/diagnostic';
import {optimize} from '../native';

export default (new Optimizer({
  async optimize({bundle, contents, logger}) {
    if (!bundle.env.shouldOptimize) {
      return {contents};
    }

    let buffer = await blobToBuffer(contents);

    // Attempt to optimize it, if the optimize fails we log a warning...
    try {
      let optimized = optimize(bundle.type, buffer);
      return {
        contents: optimized.length < buffer.length ? optimized : buffer,
      };
    } catch (err) {
      const filepath = bundle.getMainEntry()?.filePath;
      const filename = filepath
        ? path.relative(process.cwd(), filepath)
        : 'unknown';
      logger.warn({
        message: md`Could not optimize image ${filename}: ${err.message}`,
        stack: err.stack,
      });
    }

    return {contents: buffer};
  },
}): Optimizer);
Repository: parcel-bundler/parcel
File: packages/transformers/html/src/dependencies.js (JavaScript)

Code before:
        );
      });
      if (isMetaDependency) {
        const metaAssetUrl = attrs.content;
        if (metaAssetUrl) {
          attrs.content = asset.addURLDependency(attrs.content, {
            needsStableName: true,
          });
          isDirty = true;
          asset.setAST(ast);
        }
      }
      return node;
    }

    if (
      tag === 'link' &&
      (attrs.rel === 'canonical' ||
        attrs.rel === 'manifest' ||
        (attrs.rel === 'alternate' && FEED_TYPES.has(attrs.type))) &&
      attrs.href
    ) {
      let href = attrs.href;
      if (attrs.rel === 'manifest') {
        // A hack to allow manifest.json rather than manifest.webmanifest.
        // If a custom pipestart is used, it is responsible for running @parcel/transformer-webmanifest.
        if (!href.includes(':')) {
          href = 'webmanifest:' + href;
        }
      }

      attrs.href = asset.addURLDependency(href, {
        needsStableName: true,
      });
      isDirty = true;
      asset.setAST(ast);
      return node;
    }

    if (tag === 'script' && attrs.src) {
      let sourceType = attrs.type === 'module' ? 'module' : 'script';
      let loc = node.location
        ? {
            filePath: asset.filePath,
            start: node.location.start,
            end: node.location.end,
          }
        : undefined;

      let outputFormat = 'global';
      if (attrs.type === 'module' && asset.env.shouldScopeHoist) {

Review comment (suggestion):
```suggestion
        // If a custom pipeline is used, it is responsible for running @parcel/transformer-webmanifest.
```

Code after:
        );
      });
      if (isMetaDependency) {
        const metaAssetUrl = attrs.content;
        if (metaAssetUrl) {
          attrs.content = asset.addURLDependency(attrs.content, {
            needsStableName: true,
          });
          isDirty = true;
          asset.setAST(ast);
        }
      }
      return node;
    }

    if (
      tag === 'link' &&
      (attrs.rel === 'canonical' ||
        attrs.rel === 'manifest' ||
        (attrs.rel === 'alternate' && FEED_TYPES.has(attrs.type))) &&
      attrs.href
    ) {
      let href = attrs.href;
      if (attrs.rel === 'manifest') {
        // A hack to allow manifest.json rather than manifest.webmanifest.
        // If a custom pipeline is used, it is responsible for running @parcel/transformer-webmanifest.
        if (!href.includes(':')) {
          href = 'webmanifest:' + href;
        }
      }

      attrs.href = asset.addURLDependency(href, {
        needsStableName: true,
      });
      isDirty = true;
      asset.setAST(ast);
      return node;
    }

    if (tag === 'script' && attrs.src) {
      let sourceType = attrs.type === 'module' ? 'module' : 'script';
      let loc = node.location
        ? {
            filePath: asset.filePath,
            start: node.location.start,
            end: node.location.end,
          }
        : undefined;

      let outputFormat = 'global';
      if (attrs.type === 'module' && asset.env.shouldScopeHoist) {
Repository: parcel-bundler/parcel
File: packages/packagers/js/src/index.js (JavaScript)

Code before:
            options,
            bundleGraph,
            bundle,
            nullthrows(config).parcelRequireName,
          );

      ({contents, map} = await packager.package());
    }

    contents += '\n' + (await getSourceMapSuffix(getSourceMapReference, map));

    // For library builds, we need to replace URL references with their final resolved paths.
    // For non-library builds, this is handled in the JS runtime.
    if (bundle.env.isLibrary) {
      ({contents, map} = replaceURLReferences({
        bundle,
        bundleGraph,
        contents,
        map,
      }));
    }

    if (config.relativeContext) {
      const relPath = path.relative(bundle.target.distDir, options.projectRoot);
      contents = contents.replace('$parcel$dirnameReplace', relPath);
      contents = contents.replace('$parcel$filenameReplace', relPath);
    }

    return replaceInlineReferences({
      bundle,
      bundleGraph,
      contents,
      getInlineReplacement: (dependency, inlineType, content) => ({
        from: `"${dependency.id}"`,
        to: inlineType === 'string' ? JSON.stringify(content) : content,
      }),
      getInlineBundleContents,
      map,
    });
  },
}): Packager);

async function getSourceMapSuffix(
  getSourceMapReference: (?SourceMap) => Async<?string>,
  map: ?SourceMap,
): Promise<string> {
  let sourcemapReference = await getSourceMapReference(map);
  if (sourcemapReference != null) {
    return '//# sourceMappingURL=' + sourcemapReference + '\n';
  } else {
    return '';

Review comment (suggestion):
Probably need to use a regex to replace all instances. I think `replace` of a string only replaces the first one.

Code after:
            nullthrows(config).parcelRequireName,
          );

      ({contents, map} = await packager.package());
    }

    contents += '\n' + (await getSourceMapSuffix(getSourceMapReference, map));

    // For library builds, we need to replace URL references with their final resolved paths.
    // For non-library builds, this is handled in the JS runtime.
    if (bundle.env.isLibrary) {
      ({contents, map} = replaceURLReferences({
        bundle,
        bundleGraph,
        contents,
        map,
        getReplacement: s => JSON.stringify(s).slice(1, -1),
      }));
    }

    return replaceInlineReferences({
      bundle,
      bundleGraph,
      contents,
      getInlineReplacement: (dependency, inlineType, content) => ({
        from: `"${dependency.id}"`,
        to: inlineType === 'string' ? JSON.stringify(content) : content,
      }),
      getInlineBundleContents,
      map,
    });
  },
}): Packager);

async function getSourceMapSuffix(
  getSourceMapReference: (?SourceMap) => Async<?string>,
  map: ?SourceMap,
): Promise<string> {
  let sourcemapReference = await getSourceMapReference(map);
  if (sourcemapReference != null) {
    return '//# sourceMappingURL=' + sourcemapReference + '\n';
  } else {
    return '';
  }
}