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

Code before:
      additionalProperties: false,
      required: ['service_worker'],
    },
    content_security_policy: {
      type: 'object',
      properties: {
        extension_pages: string,
        sandbox: string,
      },
      additionalProperties: false,
    },
    host_permissions: arrStr,
    sandbox: {
      type: 'object',
      properties: {
        pages: arrStr,
      },
      additionalProperties: false,
    },
    side_panel: {
      type: 'object',
      properties: {
        default_path: string,
      },
    },
    web_accessible_resources: {
      type: 'array',
      items: {
        oneOf: [
          {
            ...warBase,
            required: ['resources', 'matches'],
          },
          {
            ...warBase,
            required: ['resources', 'extension_ids'],
          },
        ],
      },
    },
  },
  required: ['manifest_version', 'name', 'version'],
}: SchemaEntity);

export const MV2Schema = ({
  type: 'object',
  properties: {
    ...commonProps,
    manifest_version: {
      type: 'number',
      enum: [2],

Review comment (suggestion):
Let's add this for consistency with the other schema items

```suggestion
      additionalProperties: false,
    },
```

Code after:
      additionalProperties: false,
      required: ['service_worker'],
    },
    content_security_policy: {
      type: 'object',
      properties: {
        extension_pages: string,
        sandbox: string,
      },
      additionalProperties: false,
    },
    host_permissions: arrStr,
    sandbox: {
      type: 'object',
      properties: {
        pages: arrStr,
      },
      additionalProperties: false,
    },
    side_panel: {
      type: 'object',
      properties: {
        default_path: string,
      },
      additionalProperties: false,
    },
    web_accessible_resources: {
      type: 'array',
      items: {
        oneOf: [
          {
            ...warBase,
            required: ['resources', 'matches'],
          },
          {
            ...warBase,
            required: ['resources', 'extension_ids'],
          },
        ],
      },
    },
  },
  required: ['manifest_version', 'name', 'version'],
}: SchemaEntity);

export const MV2Schema = ({
  type: 'object',
  properties: {
    ...commonProps,
    manifest_version: {
      type: 'number',
Repository: parcel-bundler/parcel
File: packages/dev/query/src/cli.js (JavaScript)

Code before:

    let bg = {
      dependency: 0,
      bundle: 0,
      asset: 0,
      asset_node_modules: 0,
      asset_source: 0,
    };

    let bg_ext = {};

    let bg_type = {
      entries: 0,
      shared: 0,
      async: 0,
      parallel: 0,
    };

    for (let [, n] of bundleGraph._graph.nodes) {
      if (n.type == 'bundle_group') {
        // TODO: Counting priority here seems to work instead of counting on BundleNodes.. Counting DependencyNode priorities themselves
        // gives different counts as well.
        let priority = _getBundlePriority(bundleGraph, n);
        if (priority == Priority.lazy) {
          bg_type.async++;
        } else if (priority == Priority.parallel) {
          bg_type.parallel++;
        }
      } else if (n.type == 'bundle') {
        bg.bundle++;

        // $FlowFixMe
        bg_ext[n.value.type] = (bg_ext[n.value.type] || 0) + 1;

        // In general, !bundle.mainEntryId means that it is shared. Ignores async/shared edge cases.
        // $FlowFixMe
        if (n.value.mainEntryId == null) {
          bg_type.shared++;
        }
      } else if (n.type == 'asset') {
        if (
          // $FlowFixMe
          fromProjectPathRelative(n.value.filePath).includes('node_modules')
        ) {
          bg.asset_node_modules++;
        } else {
          bg.asset_source++;
        }
      } else if (n.type == 'dependency') {
        bg.dependency++;
      } else if (n.type == 'entry_file') {

Review comment (suggestion):
Should've clarified this before but BundleGroups can't be parallel, only entry or async. A bundleGroup is a collection of bundles. Connected to it there may be multiple other bundles it pulls in, like a shared bundle or a parallel (aka type change) bundle. 

So the consideration for parallel should be done in the if statement where you check for shared. That might be why your example had 0 parallel bundles.

Code after:
      columnDefault: {
        width: 18,
      },
      header: {
        alignment: 'center',
        content: header,
      },
    };

    console.log(table(data, config));
  }

  // eslint-disable-next-line no-unused-vars
  function stats(_) {
    let ag = {
      asset: 0,
      dependency: 0,
      asset_group: 0,
    };

    for (let [, n] of assetGraph.nodes) {
      if (n.type in ag) {
        // $FlowFixMe
        ag[n.type]++;
      }
    }

    let bg = {
      dependency: 0,
      bundle: 0,
      bundle_group: 0,
      asset_node_modules: 0,
      asset_source: 0,
    };

    let b_type = {
      entry: 0,
      shared: 0,
      async: 0,
      parallel: 0,
      sync: 0,
    };

    let b_ext = {};

    const entries = new Set();

    for (let [, n] of bundleGraph._graph.nodes) {
      if (n.type === 'bundle_group') {
        bg.bundle_group++;
      } else if (n.type === 'bundle') {
Repository: parcel-bundler/parcel
File: packages/core/core/src/Parcel.js (JavaScript)

Code before:
    shouldDisableCache = false,
    ...options
  }: ParcelTransformOptions): Promise<Array<Asset>> {
    if (!this.#initialized) {
      await this._init();
    }

    let projectRoot = nullthrows(this.#resolvedOptions).projectRoot;
    let request = createAssetRequest({
      ...options,
      filePath: toProjectPath(projectRoot, options.filePath),
      optionsRef: this.#optionsRef,
      env: createEnvironment({
        ...options.env,
        loc:
          options.env?.loc != null
            ? {
                ...options.env.loc,
                filePath: toProjectPath(projectRoot, options.env.loc.filePath),
              }
            : undefined,
      }),
    });

    let res = await this.#requestTracker.runRequest(request, {
      force: shouldDisableCache,
    });
    return res.map(asset =>
      assetFromValue(asset, nullthrows(this.#resolvedOptions)),
    );
  }

  async unstable_resolve({
    shouldDisableCache = false,
    ...request
  }: ParcelResolveOptions): Promise<?ParcelResolveResult> {
    if (!this.#initialized) {
      await this._init();
    }

    let projectRoot = nullthrows(this.#resolvedOptions).projectRoot;
    if (request.resolveFrom == null && path.isAbsolute(request.specifier)) {
      request.specifier = fromProjectPathRelative(
        toProjectPath(projectRoot, request.specifier),
      );
    }

    let dependency = createDependency(projectRoot, {
      ...request,
      env: createEnvironment({
        ...request.env,

Review comment (bug):
Turns out I was wrong about this and there's a separate cache layer for transforms so this is safe to hardcode as `true`.

Code after:
    options: ParcelTransformOptions,
  ): Promise<Array<Asset>> {
    if (!this.#initialized) {
      await this._init();
    }

    let projectRoot = nullthrows(this.#resolvedOptions).projectRoot;
    let request = createAssetRequest({
      ...options,
      filePath: toProjectPath(projectRoot, options.filePath),
      optionsRef: this.#optionsRef,
      env: createEnvironment({
        ...options.env,
        loc:
          options.env?.loc != null
            ? {
                ...options.env.loc,
                filePath: toProjectPath(projectRoot, options.env.loc.filePath),
              }
            : undefined,
      }),
    });

    let res = await this.#requestTracker.runRequest(request, {
      force: true,
    });
    return res.map(asset =>
      assetFromValue(asset, nullthrows(this.#resolvedOptions)),
    );
  }

  async unstable_resolve(
    request: ParcelResolveOptions,
  ): Promise<?ParcelResolveResult> {
    if (!this.#initialized) {
      await this._init();
    }

    let projectRoot = nullthrows(this.#resolvedOptions).projectRoot;
    if (request.resolveFrom == null && path.isAbsolute(request.specifier)) {
      request.specifier = fromProjectPathRelative(
        toProjectPath(projectRoot, request.specifier),
      );
    }

    let dependency = createDependency(projectRoot, {
      ...request,
      env: createEnvironment({
        ...request.env,
        loc:
          request.env?.loc != null
Repository: parcel-bundler/parcel
File: packages/core/integration-tests/test/bundler.js (JavaScript)

Code before:
      path.join(__dirname, 'integration/inlined-assests/index.js'),
      {
        mode: 'production',
        defaultTargetOptions: {
          shouldScopeHoist: false,
        },
      },
    );

    assertBundles(b, [
      {
        name: 'index.js',
        assets: [
          'index.js',
          'bundle-url.js',
          'cacheLoader.js',
          'esmodule-helpers.js',
          'js-loader.js',
          'bundle-manifest.js',
        ],
      },
      {
        assets: ['bar.js'],
      },
      {
        assets: ['foo.js'],
      },
      {
        assets: ['a.js', 'b.js', 'c.js'],
      },
      {
        assets: ['local.html'],
      },
      {
        assets: ['buzz.js'],
      },
      {
        assets: [
          'inline-module.js',
          'local.html',
          'bundle-url.js',
          'cacheLoader.js',
          'js-loader.js',
        ],
      },
      {
        assets: ['esmodule-helpers.js'],
      },
    ]);
  });


Review comment (suggestion):
I think we should make the test case smaller/less complicated by removing any bundleGroups or bundle that aren't directly related to the `inline` asset / bundleGroup we're processing.

Code after:
      path.join(__dirname, 'integration/inlined-assests/local.html'),
      {
        mode: 'production',
        defaultTargetOptions: {
          shouldScopeHoist: false,
        },
      },
    );

    assertBundles(b, [
      {
        assets: ['local.html'],
      },
      {
        assets: ['buzz.js'],
      },
      {
        assets: [
          'inline-module.js',
          'local.html',
          'bundle-url.js',
          'cacheLoader.js',
          'js-loader.js',
        ],
      },
      {
        assets: ['esmodule-helpers.js'],
      },
    ]);
  });

  it('should not create a shared bundle from an asset if that asset is shared by less than minBundles bundles', async function () {
    let b = await bundle(
      path.join(__dirname, 'integration/min-bundles/index.js'),
      {
        mode: 'production',
        defaultTargetOptions: {
          shouldScopeHoist: false,
        },
      },
    );

    assertBundles(b, [
      {
        name: 'index.js',
        assets: [
          'index.js',
          'bundle-url.js',
          'cacheLoader.js',
          'css-loader.js',
          'esmodule-helpers.js',
Repository: parcel-bundler/parcel
File: packages/packagers/js/src/ScopeHoistingPackager.js (JavaScript)

Code before:

  shouldSkipAsset(asset: Asset): boolean {
    if (this.isScriptEntry(asset)) {
      return true;
    }

    return (
      asset.sideEffects === false &&
      nullthrows(this.bundleGraph.getUsedSymbols(asset)).size == 0 &&
      !this.bundleGraph.isAssetReferenced(this.bundle, asset)
    );
  }

  isScriptEntry(asset: Asset): boolean {
    return (
      this.bundle.env.outputFormat === 'global' &&
      this.bundle.env.sourceType === 'script' &&
      asset === this.bundle.getMainEntry()
    );
  }

  buildFunctionExpression(args: Array<string>, expr: string): string {
    return this.bundle.env.supports('arrow-functions', true)
      ? `(${args.join(', ')}) => ${expr}`
      : `function (${args.join(', ')}) { return ${expr}; }`;
  }
}


Review comment (refactor):
Why has this been extracted to a `const`? (Or was this for your other optimisations you de-scoped from this change)

Code after:
      let mightBeFirstJS =
        parentBundles.length === 0 ||
        parentBundles.some(b => b.type !== 'js') ||
        this.bundleGraph
          .getBundleGroupsContainingBundle(this.bundle)
          .some(g => this.bundleGraph.isEntryBundleGroup(g)) ||
        this.bundle.env.isIsolated() ||
        this.bundle.bundleBehavior === 'isolated';

      if (mightBeFirstJS) {
        let preludeCode = prelude(this.parcelRequireName);
        res += preludeCode;
        if (enableSourceMaps) {
          lines += countLines(preludeCode) - 1;
        }

        if (this.shouldBundleQueue(this.bundle)) {
          let bundleQueuePreludeCode = bundleQueuePrelude(this.bundle.env);
          res += bundleQueuePreludeCode;
          if (enableSourceMaps) {
            lines += countLines(bundleQueuePreludeCode) - 1;
          }
        }
      } else {
        // Otherwise, get the current parcelRequire global.
        const escaped = JSON.stringify(this.parcelRequireName);
        res += `var parcelRequire = $parcel$global[${escaped}];\n`;
        lines++;
        res += `var parcelRegister = parcelRequire.register;\n`;
        lines++;
      }
    }

    // Add importScripts for sibling bundles in workers.
    if (this.bundle.env.isWorker() || this.bundle.env.isWorklet()) {
      let importScripts = '';
      let bundles = this.bundleGraph.getReferencedBundles(this.bundle);
      for (let b of bundles) {
        if (this.bundle.env.outputFormat === 'esmodule') {
          // importScripts() is not allowed in native ES module workers.
          importScripts += `import "${relativeBundlePath(this.bundle, b)}";\n`;
        } else {
          importScripts += `importScripts("${relativeBundlePath(
            this.bundle,
            b,
          )}");\n`;
        }
      }

      res += importScripts;
      lines += bundles.length;
Repository: parcel-bundler/parcel
File: packages/core/integration-tests/test/scope-hoisting.js (JavaScript)

Code before:
          `Bundle didn't inline constant values`,
        );
      }

      // Run the bundle to make sure it's valid
      await run(b);
    });
  });

  describe('commonjs', function () {
    it('should wrap when this could refer to an export', async function () {
      let b = await bundle(
        path.join(__dirname, '/integration/exports-this/a.js'),
        {
          mode: 'production',
          defaultTargetOptions: {
            shouldScopeHoist: true,
            shouldOptimize: false,
          },
        },
      );

      let contents = await outputFS.readFile(
        b.getBundles().find(b => /a\.js/.test(b.filePath)).filePath,
        'utf8',
      );

      let wrapped = contents.includes('exports.bar()');
      assert.equal(wrapped, true);
    });

    it('supports require of commonjs modules', async function () {
      let b = await bundle(
        path.join(
          __dirname,
          '/integration/scope-hoisting/commonjs/require/a.js',
        ),
      );

      let output = await run(b);
      assert.equal(output, 2);
    });

    it('concats commonjs modules in the correct order', async function () {
      let b = await bundle(
        path.join(
          __dirname,
          '/integration/scope-hoisting/commonjs/concat-order/a.js',
        ),
      );


Review comment (suggestion):
These custom build options are already set at the top of this file, and there is exactly one bundle in the test case, no need to use a regex.
```suggestion
      let b = await bundle(
        path.join(__dirname, '/integration/exports-this/a.js'),
      );

      let contents = await outputFS.readFile(
        b.getBundles()[0].filePath,
        'utf8',
      );
```

Code after:
          `Bundle didn't inline constant values`,
        );
      }

      // Run the bundle to make sure it's valid
      await run(b);
    });
  });

  describe('commonjs', function () {
    it('should wrap when this could refer to an export', async function () {
      let b = await bundle(
        path.join(
          __dirname,
          '/integration/scope-hoisting/commonjs/exports-this/a.js',
        ),
      );

      let contents = await outputFS.readFile(
        b.getBundles()[0].filePath,
        'utf8',
      );

      let wrapped = contents.includes('exports.bar()');
      assert.equal(wrapped, true);
    });

    it('supports require of commonjs modules', async function () {
      let b = await bundle(
        path.join(
          __dirname,
          '/integration/scope-hoisting/commonjs/require/a.js',
        ),
      );

      let output = await run(b);
      assert.equal(output, 2);
    });

    it('concats commonjs modules in the correct order', async function () {
      let b = await bundle(
        path.join(
          __dirname,
          '/integration/scope-hoisting/commonjs/concat-order/a.js',
        ),
      );

      let output = await run(b);
      assert.equal(output, 2);
    });
Repository: parcel-bundler/parcel
File: packages/transformers/js/core/src/collect.rs (Rust)

Code before:
      has_cjs_exports: collect.has_cjs_exports,
      is_esm: collect.is_esm,
    }
  }
}

impl Visit for Collect {
  fn visit_module(&mut self, node: &Module) {
    self.in_module_this = true;
    self.in_top_level = true;
    self.in_function = false;
    // Visit all imports first so that all imports are known when collecting used_imports
    for n in &node.body {
      if n.is_module_decl() {
        n.visit_with(self);
      }
    }
    for n in &node.body {
      if !n.is_module_decl() {
        n.visit_with(self);
      }
    }
    self.in_module_this = false;

    for (_key, node) in self.this_exprs.clone() {
      if let MemberProp::Ident(prop) = &node.prop {
        if self.exports.contains_key(&prop.sym) {
          self.should_wrap = true;
          self.add_bailout(node.span, BailoutReason::ThisInExport);
        }
      }
    }

    if let Some(bailouts) = &mut self.bailouts {
      for (key, Import { specifier, .. }) in &self.imports {
        if specifier == "*" {
          if let Some(spans) = self.non_static_access.get(key) {
            for span in spans {
              bailouts.push(Bailout {
                loc: SourceLocation::from(&self.source_map, *span),
                reason: BailoutReason::NonStaticAccess,
              })
            }
          }
        }
      }

      bailouts.sort_by(|a, b| a.loc.partial_cmp(&b.loc).unwrap());
    }
  }


Review comment (suggestion):
There are two identical `if let MemberProp::Ident(prop)` checks, one here and one below where `self.this_expr` is populated.

Code after:
      has_cjs_exports: collect.has_cjs_exports,
      is_esm: collect.is_esm,
    }
  }
}

impl Visit for Collect {
  fn visit_module(&mut self, node: &Module) {
    self.in_module_this = true;
    self.in_top_level = true;
    self.in_function = false;
    // Visit all imports first so that all imports are known when collecting used_imports
    for n in &node.body {
      if n.is_module_decl() {
        n.visit_with(self);
      }
    }
    for n in &node.body {
      if !n.is_module_decl() {
        n.visit_with(self);
      }
    }
    self.in_module_this = false;

    for (_key, (ident, span)) in std::mem::take(&mut self.this_exprs) {
      if self.exports.contains_key(&ident.sym) {
        self.should_wrap = true;
        self.add_bailout(span, BailoutReason::ThisInExport);
      }
    }

    if let Some(bailouts) = &mut self.bailouts {
      for (key, Import { specifier, .. }) in &self.imports {
        if specifier == "*" {
          if let Some(spans) = self.non_static_access.get(key) {
            for span in spans {
              bailouts.push(Bailout {
                loc: SourceLocation::from(&self.source_map, *span),
                reason: BailoutReason::NonStaticAccess,
              })
            }
          }
        }
      }

      bailouts.sort_by(|a, b| a.loc.partial_cmp(&b.loc).unwrap());
    }
  }

  collect_visit_fn!(visit_function, Function);
  collect_visit_fn!(visit_getter_prop, GetterProp);
Repository: parcel-bundler/parcel
File: packages/transformers/js/core/src/collect.rs (Rust)

Code before:
      should_wrap: collect.should_wrap,
      has_cjs_exports: collect.has_cjs_exports,
      is_esm: collect.is_esm,
    }
  }
}

impl Visit for Collect {
  fn visit_module(&mut self, node: &Module) {
    self.in_module_this = true;
    self.in_top_level = true;
    self.in_function = false;
    // Visit all imports first so that all imports are known when collecting used_imports
    for n in &node.body {
      if n.is_module_decl() {
        n.visit_with(self);
      }
    }
    for n in &node.body {
      if !n.is_module_decl() {
        n.visit_with(self);
      }
    }
    self.in_module_this = false;

    for (_key, node) in self.this_exprs.clone() {
      if let MemberProp::Ident(prop) = &node.prop {
        if self.exports.contains_key(&prop.sym) {
          self.should_wrap = true;
          self.add_bailout(node.span, BailoutReason::ThisInExport);
        }
      }
    }

    if let Some(bailouts) = &mut self.bailouts {
      for (key, Import { specifier, .. }) in &self.imports {
        if specifier == "*" {
          if let Some(spans) = self.non_static_access.get(key) {
            for span in spans {
              bailouts.push(Bailout {
                loc: SourceLocation::from(&self.source_map, *span),
                reason: BailoutReason::NonStaticAccess,
              })
            }
          }
        }
      }

      bailouts.sort_by(|a, b| a.loc.partial_cmp(&b.loc).unwrap());
    }
  }

Review comment (suggestion):
```suggestion
    for (_key, node) in &self.this_exprs {
```

Code after:
      should_wrap: collect.should_wrap,
      has_cjs_exports: collect.has_cjs_exports,
      is_esm: collect.is_esm,
    }
  }
}

impl Visit for Collect {
  fn visit_module(&mut self, node: &Module) {
    self.in_module_this = true;
    self.in_top_level = true;
    self.in_function = false;
    // Visit all imports first so that all imports are known when collecting used_imports
    for n in &node.body {
      if n.is_module_decl() {
        n.visit_with(self);
      }
    }
    for n in &node.body {
      if !n.is_module_decl() {
        n.visit_with(self);
      }
    }
    self.in_module_this = false;

    for (_key, (ident, span)) in std::mem::take(&mut self.this_exprs) {
      if self.exports.contains_key(&ident.sym) {
        self.should_wrap = true;
        self.add_bailout(span, BailoutReason::ThisInExport);
      }
    }

    if let Some(bailouts) = &mut self.bailouts {
      for (key, Import { specifier, .. }) in &self.imports {
        if specifier == "*" {
          if let Some(spans) = self.non_static_access.get(key) {
            for span in spans {
              bailouts.push(Bailout {
                loc: SourceLocation::from(&self.source_map, *span),
                reason: BailoutReason::NonStaticAccess,
              })
            }
          }
        }
      }

      bailouts.sort_by(|a, b| a.loc.partial_cmp(&b.loc).unwrap());
    }
  }

  collect_visit_fn!(visit_function, Function);