ronantakizawa/codereview-bench huggingface dataset:
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, {
    autoinstall: options.autoinstall,
    saveDev: true,
  });
}

function ensureElmIsInstalled() {
  if (!commandExists.sync('elm')) {
    throw new ThrowableDiagnostic({

Review comment (suggestion):
Ideally run the for loop directly in `transform`, or at least rename it to be lower case. This is not a class

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);
  },

  async transform({asset, options}) {
    const elmBinary = await elmBinaryPath(asset.filePath, options);
    const elm = await options.packageManager.require(
      'node-elm-compiler',
      asset.filePath,
      {
Repository: parcel-bundler/parcel
File: packages/transformers/elm/src/ElmTransformer.js (JavaScript)

Code before:

  if (elmBinary == null && !commandExists.sync('elm')) {
    throw new ThrowableDiagnostic({
      diagnostic: {
        message: "Can't find 'elm' binary.",
        hints: [
          "You can add it as an dependency for your project by running 'yarn add -D elm' or 'npm add -D elm'",
          'If you want to install it globally then follow instructions on https://elm-lang.org/',
        ],
        origin: '@parcel/elm-transformer',
      },
    });
  }

  return elmBinary;
}

async function resolveLocalElmBinary(searchPath, options) {
  try {
    let result = await options.packageManager.resolve(
      'elm/package.json',
      searchPath,
      {autoinstall: false},
    );

    let bin = nullthrows(result.pkg).bin;
    return path.join(
      path.dirname(result.resolved),
      typeof bin === 'string' ? bin : bin.elm,
    );
  } catch (_) {
    return null;
  }
}

function compileToString(elm, elmBinary, asset, config) {
  return elm.compileToString(asset.filePath, {
    cwd: config.cwd,
    debug: config.debug,
    optimize: config.optimize,
    pathToElm: elmBinary,
  });
}

async function injectHotModuleReloadRuntime(code, filePath, options) {
  const elmHMR = await options.packageManager.require('elm-hot', filePath, {
    autoinstall: options.autoinstall,
    saveDev: true,
  });
  return elmHMR.inject(code);
}

Review comment (suggestion):
```suggestion
    let bin = nullthrows(result.pkg?.bin);
```

Code after:
    let code = await compileToString(elm, elmBinary, asset, compilerConfig);
    if (options.hot) {
      code = await injectHotModuleReloadRuntime(code, asset.filePath, options);
    }
    if (compilerConfig.optimize) code = await minifyElmOutput(code);

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

async function elmBinaryPath(searchPath, options) {
  let elmBinary = await resolveLocalElmBinary(searchPath, options);

  if (elmBinary == null && !commandExists.sync('elm')) {
    throw new ThrowableDiagnostic({
      diagnostic: {
        message: "Can't find 'elm' binary.",
        hints: [
          "You can add it as an dependency for your project by running 'yarn add -D elm' or 'npm add -D elm'",
          'If you want to install it globally then follow instructions on https://elm-lang.org/',
        ],
        origin: '@parcel/elm-transformer',
      },
    });
  }

  return elmBinary;
}

async function resolveLocalElmBinary(searchPath, options) {
  try {
    let result = await options.packageManager.resolve(
      'elm/package.json',
      searchPath,
      {autoinstall: false},
    );

    let bin = nullthrows(result.pkg?.bin);
    return path.join(
      path.dirname(result.resolved),
      typeof bin === 'string' ? bin : bin.elm,
    );
  } catch (_) {
    return null;
  }
}

function compileToString(elm, elmBinary, asset, config) {
  return elm.compileToString(asset.filePath, {
Repository: parcel-bundler/parcel
File: packages/transformers/jsonld/src/JSONLDTransformer.js (JavaScript)

Code before:
  'logo',
  'photo',
  'image',
  'thumbnail',
  'screenshot',
  'primaryImageOfPage',
  'embedUrl',
  'thumbnailUrl',
  'video',
  'contentUrl',
  'url'
];

export default new Transformer({
  async transform({ asset, config, logger, options, resolve }) {
    // allowing any recieved jsonld to be in json5 format
    let rawCode = await asset.getCode();
    let jsonCode = json5.parse(rawCode);

    /* console.log(jsonCode);
    console.log(config);
    console.log(options);
    console.log(JSON.stringify({ asset, config, logger, options, resolve })); */
    
    console.log(jsonCode);
    let parser = new JSONLDParser(asset);
    console.log("parsing....");
    parser.parse(jsonCode);
    console.log("done");
    console.log(jsonCode);
    
    // this will send the jsonld to the JSONTransformer
    asset.type = 'json';
    // setting it to jsonCode since the parser updates asset paths
    asset.setCode(JSON.stringify(jsonCode));
    return [asset];
  },
});

class JSONLDParser {
  asset;
  publicURL;
  constructor(asset){
    this.asset = asset;    
    this.publicURL = ""; // unable to figure out where I can get this from
  }

  parse(jsonld) {
    jsonld = this.extractUrlsFrom(jsonld);
  }


Review comment (suggestion):
- parcel v1 seems to inject the json object write back into the script tag on the html page
- The only difference is that any asset paths that was in the jsonld are now updated with the new asset paths
- However, my local parcel injects the json object wrapped in javascript code with `module.exports=` and syntax like that

Code after:
  'photo',
  'image',
  'thumbnail',
  'screenshot',
  'primaryImageOfPage',
  'embedUrl',
  'thumbnailUrl',
  'video',
  'contentUrl',
  'url',
];

export default new Transformer({
  async transform({asset}) {
    let rawCode = await asset.getCode();
    // allowing any recieved jsonld to be in json5 format
    let jsonCode = json5.parse(rawCode);

    jsonCode = extractUrlsFrom(jsonCode, asset);

    // json should be injected back into the html page
    asset.type = 'jsonld';
    // setting it to jsonCode since the parser updates asset paths
    asset.setCode(JSON.stringify(jsonCode));
    return [asset];
  },
});

function extractUrlsFrom(data, asset) {
  if (!data) return null;

  if (typeof data === 'string') {
    let assetPath = asset.addURLDependency(data, {});
    return assetPath;
  }

  if (Array.isArray(data)) return iterateThroughArray(data, asset);

  return iterateThroughObject(data, asset);
}

function iterateThroughObject(jsonObject, asset) {
  Object.keys(jsonObject)
    .filter(k => SCHEMA_ATTRS.includes(k))
    .forEach(k => {
      let value = jsonObject[k];
      jsonObject[k] = extractUrlsFrom(value, asset);
    });

  return jsonObject;
}
Repository: parcel-bundler/parcel
File: packages/transformers/jsonld/src/JSONLDTransformer.js (JavaScript)

Code before:
// @flow

import {Transformer} from '@parcel/plugin';
import json5 from 'json5';
import { isURL, urlJoin } from '@parcel/utils';

// A list of all attributes in a schema that may produce a dependency
// Based on https://schema.org/ImageObject
// Section "Instances of ImageObject may appear as values for the following properties"
const SCHEMA_ATTRS = [
  'logo',
  'photo',
  'image',
  'thumbnail',
  'screenshot',
  'primaryImageOfPage',
  'embedUrl',
  'thumbnailUrl',
  'video',
  'contentUrl',
  'url'
];

export default new Transformer({
  async transform({ asset, config, logger, options, resolve }) {
    // allowing any recieved jsonld to be in json5 format
    let rawCode = await asset.getCode();
    let jsonCode = json5.parse(rawCode);

    /* console.log(jsonCode);
    console.log(config);
    console.log(options);
    console.log(JSON.stringify({ asset, config, logger, options, resolve })); */
    
    console.log(jsonCode);
    let parser = new JSONLDParser(asset);
    console.log("parsing....");
    parser.parse(jsonCode);
    console.log("done");
    console.log(jsonCode);
    
    // this will send the jsonld to the JSONTransformer
    asset.type = 'json';
    // setting it to jsonCode since the parser updates asset paths
    asset.setCode(JSON.stringify(jsonCode));
    return [asset];
  },
});

class JSONLDParser {
  asset;

Review comment (suggestion):
from which of the available object (config or options?) would I be able to get the `publicURL`? Whenever I try to reference it like `option.publicURL` I get a compiler error from flow

Code after:
// @flow

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

// A list of all attributes in a schema that may produce a dependency
// Based on https://schema.org/ImageObject
// Section "Instances of ImageObject may appear as values for the following properties"
const SCHEMA_ATTRS = [
  'logo',
  'photo',
  'image',
  'thumbnail',
  'screenshot',
  'primaryImageOfPage',
  'embedUrl',
  'thumbnailUrl',
  'video',
  'contentUrl',
  'url',
];

export default new Transformer({
  async transform({asset}) {
    let rawCode = await asset.getCode();
    // allowing any recieved jsonld to be in json5 format
    let jsonCode = json5.parse(rawCode);

    jsonCode = extractUrlsFrom(jsonCode, asset);

    // json should be injected back into the html page
    asset.type = 'jsonld';
    // setting it to jsonCode since the parser updates asset paths
    asset.setCode(JSON.stringify(jsonCode));
    return [asset];
  },
});

function extractUrlsFrom(data, asset) {
  if (!data) return null;

  if (typeof data === 'string') {
    let assetPath = asset.addURLDependency(data, {});
    return assetPath;
  }

  if (Array.isArray(data)) return iterateThroughArray(data, asset);

  return iterateThroughObject(data, asset);
}
Repository: parcel-bundler/parcel
File: packages/reporters/sourcemap-visualiser/src/SourceMapVisualiser.js (JavaScript)

Code before:
                  sourceContent = await options.inputFS.readFile(
                    path.resolve(options.projectRoot, s),
                    'utf-8',
                  );
                } catch (e) {
                  logger.warn({
                    message: `Error while loading content of ${s}, ${e.message}`,
                  });
                }

                return {
                  name: s,
                  content: sourceContent,
                };
              }),
            );

            let relativePath = relatifyPath(options.projectRoot, p);
            bundles.push({
              name: relativePath,
              mappings: map.mappings,
              names: map.names,
              sources: mappedSources,
              // $FlowFixMe
              content: await options.outputFS.readFile(
                bundle.filePath,
                'utf-8',
              ),
            });
          }
        }
      }

      await options.outputFS.writeFile(
        path.join(options.projectRoot, 'sourcemap-info.json'),
        JSON.stringify(bundles),
      );

      logger.log({
        message: `Goto https://sourcemap-visualiser.now.sh/ and upload the generated sourcemap-info.json file to visualise and debug the sourcemaps.`,
      });
    }
  },
});


Review comment (suggestion):
`nullthrows(bundle.filePath)` instead of the flow comment

Code after:
                try {
                  sourceContent = await options.inputFS.readFile(
                    path.resolve(options.projectRoot, s),
                    'utf-8',
                  );
                } catch (e) {
                  logger.warn({
                    message: `Error while loading content of ${s}, ${e.message}`,
                  });
                }

                return {
                  name: s,
                  content: sourceContent,
                };
              }),
            );

            let relativePath = relatifyPath(options.projectRoot, p);
            bundles.push({
              name: relativePath,
              mappings: map.mappings,
              names: map.names,
              sources: mappedSources,
              content: await options.outputFS.readFile(
                nullthrows(bundle.filePath),
                'utf-8',
              ),
            });
          }
        }
      }

      await options.outputFS.writeFile(
        path.join(options.projectRoot, 'sourcemap-info.json'),
        JSON.stringify(bundles),
      );

      logger.log({
        message: `Goto https://sourcemap-visualiser.now.sh/ and upload the generated sourcemap-info.json file to visualise and debug the sourcemaps.`,
      });
    }
  },
});
Repository: parcel-bundler/parcel
File: packages/core/core/src/public/Dependency.js (JavaScript)

Code before:
  IDependency,
  InternalDependency,
> = new WeakMap();
export function dependencyToInternalDependency(
  dependency: IDependency,
): InternalDependency {
  return nullthrows(_dependencyToInternalDependency.get(dependency));
}

export default class Dependency implements IDependency {
  #dep; // InternalDependency

  constructor(dep: InternalDependency) {
    let existing = internalDependencyToDependency.get(dep);
    if (existing != null) {
      return existing;
    }

    this.#dep = dep;
    _dependencyToInternalDependency.set(this, dep);
    internalDependencyToDependency.set(dep, this);
  }

  // $FlowFixMe
  [inspect]() {
    return `Dependency(${this.sourcePath} -> ${this.filePath})`;
  }

  get id(): string {
    return this.#dep.id;
  }

  get moduleSpecifier(): string {
    return this.#dep.moduleSpecifier;
  }

  get isAsync(): boolean {
    return !!this.#dep.isAsync;
  }

  get isEntry(): boolean {
    return !!this.#dep.isEntry;
  }

  get isOptional(): boolean {
    return !!this.#dep.isOptional;
  }

  get isURL(): boolean {
    return !!this.#dep.isURL;
  }

Review comment (suggestion):
This is great. Can it include the `moduleSpecifier`?

Maybe we could use these when generating node labels in graphviz 🤔 

Code after:
  IDependency,
  InternalDependency,
> = new WeakMap();
export function dependencyToInternalDependency(
  dependency: IDependency,
): InternalDependency {
  return nullthrows(_dependencyToInternalDependency.get(dependency));
}

export default class Dependency implements IDependency {
  #dep; // InternalDependency

  constructor(dep: InternalDependency) {
    let existing = internalDependencyToDependency.get(dep);
    if (existing != null) {
      return existing;
    }

    this.#dep = dep;
    _dependencyToInternalDependency.set(this, dep);
    internalDependencyToDependency.set(dep, this);
  }

  // $FlowFixMe
  [inspect]() {
    return `Dependency(${String(this.sourcePath)} -> ${this.moduleSpecifier})`;
  }

  get id(): string {
    return this.#dep.id;
  }

  get moduleSpecifier(): string {
    return this.#dep.moduleSpecifier;
  }

  get isAsync(): boolean {
    return !!this.#dep.isAsync;
  }

  get isEntry(): boolean {
    return !!this.#dep.isEntry;
  }

  get isOptional(): boolean {
    return !!this.#dep.isOptional;
  }

  get isURL(): boolean {
    return !!this.#dep.isURL;
  }
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({
  getConfig({options}) {
    return Promise.resolve({
      stripIgnoredCharacters: options.mode === 'production',
    });
  },

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

    const expandedDocument = await processDocumentImports(document, loadImport);

    const generated =
      config && config.stripIgnoredCharacters
        ? stripIgnoredCharacters(print(expandedDocument))
        : print(expandedDocument);

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

    return [asset];

    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),
      );
    }
  },
});


Review comment (bug):
You can just access `options` directly here, using `getConfig` provides no benefit in this case.

As I already said in the linked issue, `asset.env.minify` fits better here than `mode`, this way specifying `--no-minify` on the CLI might help debugging.

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});

      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];
  },
});
Repository: parcel-bundler/parcel
File: packages/transformers/graphql/src/GraphQLTransformer.js (JavaScript)

Code before:
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({
  getConfig({options}) {
    return Promise.resolve({
      stripIgnoredCharacters: options.mode === 'production',
    });
  },

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

    const expandedDocument = await processDocumentImports(document, loadImport);

    const generated =
      config && config.stripIgnoredCharacters
        ? stripIgnoredCharacters(print(expandedDocument))
        : print(expandedDocument);

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

    return [asset];

    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),
      );
    }
  },
});


Review comment (refactor):
Please move this function to the bottom of the file or before the return (e.g. near `processDocumentImports`). 

Code after:
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});

      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];
  },
});