ronantakizawa/codereview-bench huggingface dataset:
Repository: parcel-bundler/parcel
File: packages/core/core/src/requests/ConfigRequest.js (JavaScript)

Code before:
        origin: loadedPlugin.name,
      }),
    });
  }
}

const packageKeyCache = createBuildCache();

export async function getPackageKeyContentHash(
  filePath: ProjectPath,
  packageKey: string,
  options: ParcelOptions,
): Async<string> {
  let cacheKey = `${fromProjectPathRelative(filePath)}:${packageKey}`;
  let cachedValue = packageKeyCache.get(cacheKey);

  if (cachedValue) {
    return cachedValue;
  }

  let conf = await readConfig(
    options.inputFS,
    fromProjectPath(options.projectRoot, filePath),
  );

  if (conf == null || conf.config[packageKey] == null) {
    throw new Error(
      `Expected config to exist: '${fromProjectPathRelative(filePath)}'`,
    );
  }

  let contentHash =
    typeof conf.config[packageKey] === 'object'
      ? hashObject(conf.config[packageKey])
      : hashString(JSON.stringify(conf.config[packageKey]));

  packageKeyCache.set(cacheKey, contentHash);

  return contentHash;
}

export async function runConfigRequest<TResult>(
  api: RunAPI<TResult>,
  configRequest: ConfigRequest,
) {
  let {
    invalidateOnFileChange,
    invalidateOnPackageKeyChange,
    invalidateOnFileCreate,
    invalidateOnEnvChange,
    invalidateOnOptionChange,

Review comment (suggestion):
The `packageKey` being removed could be a valid scenario couldn't it? Like if I remove custom config for a transformer that I no longer need because the defaults are file.

Code after:
      diagnostic: errorToDiagnostic(e, {
        origin: loadedPlugin.name,
      }),
    });
  }
}

const configKeyCache = createBuildCache();

export async function getConfigKeyContentHash(
  filePath: ProjectPath,
  configKey: string,
  options: ParcelOptions,
): Async<string> {
  let cacheKey = `${fromProjectPathRelative(filePath)}:${configKey}`;
  let cachedValue = configKeyCache.get(cacheKey);

  if (cachedValue) {
    return cachedValue;
  }

  let conf = await readConfig(
    options.inputFS,
    fromProjectPath(options.projectRoot, filePath),
  );

  if (conf == null || conf.config[configKey] == null) {
    // This can occur when a config key has been removed entirely during `respondToFSEvents`
    return '';
  }

  let contentHash =
    typeof conf.config[configKey] === 'object'
      ? hashObject(conf.config[configKey])
      : hashString(JSON.stringify(conf.config[configKey]));

  configKeyCache.set(cacheKey, contentHash);

  return contentHash;
}

export async function runConfigRequest<TResult>(
  api: RunAPI<TResult>,
  configRequest: ConfigRequest,
) {
  let {
    invalidateOnFileChange,
    invalidateOnConfigKeyChange,
    invalidateOnFileCreate,
    invalidateOnEnvChange,
    invalidateOnOptionChange,
Repository: parcel-bundler/parcel
File: packages/core/core/src/RequestTracker.js (JavaScript)

Code before:
        input: request.input,
        api,
        farm: this.farm,
        options: this.options,
        invalidateReason: node.invalidateReason,
      });

      assertSignalNotAborted(this.signal);
      this.completeRequest(requestNodeId);

      deferred.resolve(true);
      return result;
    } catch (err) {
      this.rejectRequest(requestNodeId);
      deferred.resolve(false);
      throw err;
    } finally {
      this.graph.replaceSubrequests(requestNodeId, [...subRequestContentKeys]);
    }
  }

  flushStats() {
    let requestTypeEntries = {};
    type RequestTypes = Array<$Keys<typeof requestTypes>>;

    for (let key of (Object.keys(requestTypes): RequestTypes)) {
      requestTypeEntries[requestTypes[key]] = key;
    }

    let formattedStats = {};
    let messageLines = ['Request stats'];

    for (let [requestType, count] of this.stats.entries()) {
      let requestTypeName = requestTypeEntries[requestType];
      formattedStats[requestTypeName] = count;
      messageLines.push(`${requestTypeName}\t\t${count}`);
    }

    let message = messageLines.join('\n');

    logger.verbose({
      origin: '@parcel/core',
      message,
      meta: {
        requestStats: formattedStats,
      },
    });

    this.stats = new Map();
  }


Review comment (suggestion):
/nit you could just create this mapping at the top of the file so it's done once, and it could be more idiomatic using `Object.entries`

Code after:
      let result = await request.run({
        input: request.input,
        api,
        farm: this.farm,
        options: this.options,
        invalidateReason: node.invalidateReason,
      });

      assertSignalNotAborted(this.signal);
      this.completeRequest(requestNodeId);

      deferred.resolve(true);
      return result;
    } catch (err) {
      this.rejectRequest(requestNodeId);
      deferred.resolve(false);
      throw err;
    } finally {
      this.graph.replaceSubrequests(requestNodeId, [...subRequestContentKeys]);
    }
  }

  flushStats(): {[requestType: string]: number} {
    let requestTypeEntries = {};

    for (let key of (Object.keys(requestTypes): RequestTypeName[])) {
      requestTypeEntries[requestTypes[key]] = key;
    }

    let formattedStats = {};

    for (let [requestType, count] of this.stats.entries()) {
      let requestTypeName = requestTypeEntries[requestType];
      formattedStats[requestTypeName] = count;
    }

    this.stats = new Map();

    return formattedStats;
  }

  createAPI<TResult>(
    requestId: NodeId,
    previousInvalidations: Array<RequestInvalidation>,
  ): {|api: RunAPI<TResult>, subRequestContentKeys: Set<ContentKey>|} {
    let subRequestContentKeys = new Set<ContentKey>();
    let api: RunAPI<TResult> = {
      invalidateOnFileCreate: input =>
        this.graph.invalidateOnFileCreate(requestId, input),
      invalidateOnConfigKeyChange: (filePath, configKey, contentHash) =>
        this.graph.invalidateOnConfigKeyChange(
Repository: parcel-bundler/parcel
File: packages/core/graph/src/Graph.js (JavaScript)

Code before:
// @flow strict-local

import {fromNodeId} from './types';
import AdjacencyList, {type SerializedAdjacencyList} from './AdjacencyList';
import type {Edge, NodeId} from './types';
import type {
  TraversalActions,
  GraphVisitor,
  GraphTraversalCallback,
} from '@parcel/types';
import {BitSet} from './BitSet';

import nullthrows from 'nullthrows';
import {getFeatureFlag} from '@parcel/feature-flags/src';

export type NullEdgeType = 1;
export type GraphOpts<TNode, TEdgeType: number = 1> = {|
  nodes?: Array<TNode | null>,
  adjacencyList?: SerializedAdjacencyList<TEdgeType>,
  rootNodeId?: ?NodeId,
|};

export type SerializedGraph<TNode, TEdgeType: number = 1> = {|
  nodes: Array<TNode | null>,
  adjacencyList: SerializedAdjacencyList<TEdgeType>,
  rootNodeId: ?NodeId,
|};

export type AllEdgeTypes = -1;
export const ALL_EDGE_TYPES: AllEdgeTypes = -1;

type DFSCommandVisit<TContext> = {|
  nodeId: NodeId,
  context: TContext | null,
|};

type DFSCommandExit<TContext> = {|
  nodeId: NodeId,
  exit: GraphTraversalCallback<NodeId, TContext>,

Review comment (suggestion):
```suggestion
import {getFeatureFlag} from '@parcel/feature-flags';
```

Code after:
// @flow strict-local

import {fromNodeId} from './types';
import AdjacencyList, {type SerializedAdjacencyList} from './AdjacencyList';
import type {Edge, NodeId} from './types';
import {getFeatureFlag} from '@parcel/feature-flags';
import type {
  TraversalActions,
  GraphVisitor,
  GraphTraversalCallback,
} from '@parcel/types';
import {BitSet} from './BitSet';

import nullthrows from 'nullthrows';

export type NullEdgeType = 1;
export type GraphOpts<TNode, TEdgeType: number = 1> = {|
  nodes?: Array<TNode | null>,
  adjacencyList?: SerializedAdjacencyList<TEdgeType>,
  rootNodeId?: ?NodeId,
|};

export type SerializedGraph<TNode, TEdgeType: number = 1> = {|
  nodes: Array<TNode | null>,
  adjacencyList: SerializedAdjacencyList<TEdgeType>,
  rootNodeId: ?NodeId,
|};

export type AllEdgeTypes = -1;
export const ALL_EDGE_TYPES: AllEdgeTypes = -1;

type DFSCommandVisit<TContext> = {|
  nodeId: NodeId,
  context: TContext | null,
|};

type DFSCommandExit<TContext> = {|
  nodeId: NodeId,
  exit: GraphTraversalCallback<NodeId, TContext>,
Repository: parcel-bundler/parcel
File: packages/core/cache/src/FSCache.js (JavaScript)

Code before:
                  i * WRITE_LIMIT_CHUNK,
                  (i + 1) * WRITE_LIMIT_CHUNK,
                )
              : contents.subarray(
                  i * WRITE_LIMIT_CHUNK,
                  (i + 1) * WRITE_LIMIT_CHUNK,
                ),
            {signal: options?.signal},
          ),
        );
      }
    }

    // If there's already a files following this chunk, it's old and should be removed
    writePromises.push(this.#unlinkChunks(key, chunks));

    await Promise.all(writePromises);
  }

  async get<T>(key: string): Promise<?T> {
    try {
      let data = await this.fs.readFile(this._getCachePath(key));
      return deserialize(data);
    } catch (err) {
      if (err.code === 'ENOENT') {
        return null;
      } else {
        throw err;
      }
    }
  }

  async set(key: string, value: mixed): Promise<void> {
    try {
      let blobPath = this._getCachePath(key);
      let data = serialize(value);

      await this.fs.writeFile(blobPath, data);
    } catch (err) {
      logger.error(err, '@parcel/cache');
    }
  }

  refresh(): void {
    // NOOP
  }
}

registerSerializableClass(`${packageJson.version}:FSCache`, FSCache);


Review comment (bug):
/nit `fs.exists` (as opposed to `existsSync`) prior to performing a file operation is "[deprecated](https://nodejs.org/api/fs.html#fsexistspath-callback)" as the existence could change between the two operations.

It's an edge case, but you should probably use `existsSync` here. I believe `rimraf` itself will never "fail" if the file doesn't exist.

Code after:
                  i * WRITE_LIMIT_CHUNK,
                  (i + 1) * WRITE_LIMIT_CHUNK,
                )
              : contents.subarray(
                  i * WRITE_LIMIT_CHUNK,
                  (i + 1) * WRITE_LIMIT_CHUNK,
                ),
            {signal: options?.signal},
          ),
        );
      }
    }

    // If there's already a files following this chunk, it's old and should be removed
    writePromises.push(this.#unlinkChunks(key, chunks));

    await Promise.all(writePromises);
  }

  async deleteLargeBlob(key: string): Promise<void> {
    const deletePromises: Promise<void>[] = [];

    let i = 0;
    let filePath = this.#getFilePath(key, i);

    while (await this.fs.exists(filePath)) {
      deletePromises.push(this.fs.rimraf(filePath));
      i += 1;
      filePath = this.#getFilePath(key, i);
    }

    await Promise.all(deletePromises);
  }

  async get<T>(key: string): Promise<?T> {
    try {
      let data = await this.fs.readFile(this._getCachePath(key));
      return deserialize(data);
    } catch (err) {
      if (err.code === 'ENOENT') {
        return null;
      } else {
        throw err;
      }
    }
  }

  async set(key: string, value: mixed): Promise<void> {
    try {
      let blobPath = this._getCachePath(key);
      let data = serialize(value);
Repository: parcel-bundler/parcel
File: packages/core/integration-tests/test/bundler.js (JavaScript)

Code before:
        export const value = 'async value';

      package.json:
        {
          "@parcel/transformer-js": {
            "unstable_inlineConstants": true
          }
        }

      yarn.lock:`;

    let b = await bundle(
      path.join(__dirname, 'inline-constants-async/index.js'),
      {
        mode: 'production',
        defaultTargetOptions: {
          shouldScopeHoist: true,
          sourceMaps: false,
          shouldOptimize: false,
        },
        inputFS: overlayFS,
      },
    );

    // This will fail when the async bundle does not export it's constant
    await run(b);
  });
  describe('manual shared bundles', () => {
    const dir = path.join(__dirname, 'manual-bundle');

    beforeEach(() => {
      overlayFS.mkdirp(dir);
    });

    afterEach(() => {
      overlayFS.rimraf(dir);
    });

    it('should support manual shared bundles via glob config option for different types', async function () {
      await fsFixture(overlayFS, dir)`
      yarn.lock:
        // Required for config loading
      package.json:
        {
          "@parcel/bundler-default": {
            "minBundleSize": 0,
            "manualSharedBundles": [{
              "name": "vendor",
              "assets": ["vendor*.*"]
            }]
          }

Review comment (suggestion):
Does the asset still get inlined in this case? It wouldn't be ideal if you ended up with something like:
```js
import('./async').then(m => console.log('async value'));
```

Code after:
        export const value = 'async value';

      package.json:
        {
          "@parcel/transformer-js": {
            "unstable_inlineConstants": true
          }
        }

      yarn.lock:`;

    let b = await bundle(
      path.join(__dirname, 'inline-constants-async/index.js'),
      {
        mode: 'production',
        defaultTargetOptions: {
          shouldScopeHoist: true,
          sourceMaps: false,
          shouldOptimize: false,
        },
        inputFS: overlayFS,
      },
    );

    // This will fail when the async bundle does not export it's constant
    await run(b);

    // Asset should not be inlined
    const index = b.getBundles().find(b => b.name.startsWith('index'));
    const contents = overlayFS.readFileSync(index.filePath, 'utf8');
    assert(
      !contents.includes('async value'),
      'async value should not be inlined',
    );
  });
  describe('manual shared bundles', () => {
    const dir = path.join(__dirname, 'manual-bundle');

    beforeEach(() => {
      overlayFS.mkdirp(dir);
    });

    afterEach(() => {
      overlayFS.rimraf(dir);
    });

    it('should support manual shared bundles via glob config option for different types', async function () {
      await fsFixture(overlayFS, dir)`
      yarn.lock:
        // Required for config loading
      package.json:
Repository: parcel-bundler/parcel
File: packages/utils/parcel-watcher-watchman-js/src/wrapper.js (JavaScript)

Code before:
// Matches the Watcher API from "@parcel/watcher"
export interface Watcher {
  getEventsSince(
    dir: FilePath,
    snapshot: FilePath,
    opts?: Options,
  ): Promise<Array<Event>>;
  subscribe(
    dir: FilePath,
    fn: SubscribeCallback,
    opts?: Options,
  ): Promise<AsyncSubscription>;
  unsubscribe(
    dir: FilePath,
    fn: SubscribeCallback,
    opts?: Options,
  ): Promise<void>;
  writeSnapshot(
    dir: FilePath,
    snapshot: FilePath,
    opts?: Options,
  ): Promise<FilePath>;
}

let client: watchman.Client | null = null;
function getClient() {
  if (!client) {
    client = new watchman.Client();
  }
  return client;
}

function commandAsync(args: any[]): Promise<any> {
  return new Promise((resolve, reject) => {
    const client = getClient();
    // $FlowFixMe
    client.command(args, (err: Error | null | undefined, response: any) => {
      if (err) reject(err);
      else resolve(response);
    });
  });
}

class ParcelWatcherWatchmanJS implements Watcher {
  subscriptionName: string;

  constructor() {
    this.subscriptionName = 'parcel-watcher-subscription-' + Date.now();
  }

  // Types should match @parcel/watcher/index.js.flow

Review comment (refactor):
Can't we just init this in the `ParcelWatcherWatchmanJS` constructor and create a helper for the commands?

Code after:

// Matches the Watcher API from "@parcel/watcher"
export interface Watcher {
  getEventsSince(
    dir: FilePath,
    snapshot: FilePath,
    opts?: Options,
  ): Promise<Array<Event>>;
  subscribe(
    dir: FilePath,
    fn: SubscribeCallback,
    opts?: Options,
  ): Promise<AsyncSubscription>;
  unsubscribe(
    dir: FilePath,
    fn: SubscribeCallback,
    opts?: Options,
  ): Promise<void>;
  writeSnapshot(
    dir: FilePath,
    snapshot: FilePath,
    opts?: Options,
  ): Promise<FilePath>;
}

export class ParcelWatcherWatchmanJS implements Watcher {
  subscriptionName: string;
  client: watchman.Client;

  constructor() {
    this.subscriptionName = 'parcel-watcher-subscription-' + Date.now();
    this.client = new watchman.Client();
  }

  commandAsync(args: any[]): Promise<any> {
    return new Promise((resolve, reject) => {
      const client = this.client;
      // $FlowFixMe
      client.command(args, (err: Error | null | undefined, response: any) => {
        if (err) reject(err);
        else resolve(response);
      });
    });
  }

  // Types should match @parcel/watcher/index.js.flow
  async writeSnapshot(dir: string, snapshot: FilePath): Promise<string> {
    const response = await this.commandAsync(['clock', dir]);
    fs.writeFileSync(snapshot, response.clock, {
      encoding: 'utf-8',
    });
Repository: parcel-bundler/parcel
File: crates/parcel_core/src/plugin/transformer_plugin.rs (Rust)

Code before:
  /// Transform the asset and/or add new assets
  fn transform(
    &mut self,
    config: &PluginConfig,
    asset: &mut Asset,
    resolve: &Resolve,
  ) -> Result<Vec<Asset>, anyhow::Error>;

  // Perform processing after the transformation
  fn post_process(
    &mut self,
    config: &PluginConfig,
    assets: Vec<&Asset>,
  ) -> Result<Vec<Asset>, anyhow::Error>;

  /// Stringify the AST
  ///
  /// This function is called when the next transformer AST cannot be reused, or this is the last
  /// transformer in a pipeline.
  ///
  fn generate(&self, asset: Asset, ast: AST) -> Result<GenerateOutput, anyhow::Error>;
}

#[cfg(test)]
mod tests {
  use super::*;

  #[derive(Debug)]
  struct TestTransformerPlugin {}

  impl TransformerPlugin for TestTransformerPlugin {
    fn can_reuse_ast(&self, _ast: AST) -> bool {
      todo!()
    }

    fn parse(
      &mut self,
      _config: &PluginConfig,
      _asset: &Asset,
      _resolve: &Resolve,
    ) -> Result<AST, anyhow::Error> {
      todo!()
    }

    fn transform(
      &mut self,
      _config: &PluginConfig,
      _asset: &mut Asset,
      _resolve: &Resolve,
    ) -> Result<Vec<Asset>, anyhow::Error> {
      todo!()

Review comment (suggestion):
I personally find the existing invalidations API tedious to work with and don't think it should be modelled like this long-term. There is a common `Invalidation` type that the request tracker works off (though it is currently a stub), which plugins could return and it would enable all types of invalidations. Whether we want to scope them down per plugin is a different question though. Anyways, just food for thought.

Code after:
    }
  }
}

/// Context parameters for the transformer, other than the input.
pub struct RunTransformContext {
  file_system: FileSystemRef,
}

impl RunTransformContext {
  pub fn new(file_system: FileSystemRef) -> Self {
    Self { file_system }
  }

  pub fn file_system(&self) -> FileSystemRef {
    self.file_system.clone()
  }
}

#[derive(Debug)]
pub struct TransformResult {
  pub asset: Asset,
  pub dependencies: Vec<Dependency>,
  /// The transformer signals through this field that its result should be invalidated
  /// if these paths change.
  pub invalidate_on_file_change: Vec<PathBuf>,
}

/// Compile a single asset, discover dependencies, or convert the asset to a different format
///
/// Many transformers are wrappers around other tools such as compilers and preprocessors, and are
/// designed to integrate with Parcel.
///
pub trait TransformerPlugin: Debug + Send + Sync {
  /// Transform the asset and/or add new assets
  fn transform(
    &mut self,
    context: &mut RunTransformContext,
    input: TransformationInput,
  ) -> Result<TransformResult, anyhow::Error>;
}
Repository: parcel-bundler/parcel
File: packages/transformers/js/core/src/hoist.rs (Rust)

Code before:
  /// ```skip
  /// const $abc$export$hashfashdfasdfahsdfh_x = 1234;
  /// function $abc$export$hashfashdfasdfahsdfh_something() {}
  /// ```
  ///
  ///
  /// This `exported_symbols` vector will be:
  ///
  /// ```skip
  /// vec![
  ///     ExportedSymbol {
  ///         exported: "x",
  ///         local: "$abc$export$hashfashdfashdfahsdfh_x",
  ///         ...
  ///     },
  ///     ExportedSymbol {
  ///         exported: "something",
  ///         local: "$abc$export$hashfashdfashdfahsdfh_something",
  ///         ...
  ///     },
  /// ]
  /// ```
  pub exported_symbols: Vec<ExportedSymbol>,
  /// Symbols re-exported from other modules.
  ///
  /// If a symbol is re-exported from another module, parcel will remove delete the export statement
  /// from the asset.
  ///
  /// For example, if an input file is:
  ///
  /// ```skip
  /// export { view as mainView } from './view';
  /// ```
  ///
  /// The output will be
  /// ```skip
  /// import 'abc:./view:esm';
  /// ```
  ///
  /// And this vector will contain the information about the re-exported symbol.
  ///
  /// On this case, the fields of `ImportedSymbol` will mean different things than they do for
  /// [`HoistResult::imported_symbols`].
  ///
  /// In particular, since there is no mangled name, `local` means the "exported" name rather than
  /// the mangled name.
  ///
  /// On the case above, this field would be:
  ///
  /// ```skip
  /// vec![

Review comment (suggestion):
```suggestion
  /// If a symbol is re-exported from another module, parcel will remove the export statement
```

Code after:
  /// ```skip
  /// const $abc$export$hashfashdfasdfahsdfh_x = 1234;
  /// function $abc$export$hashfashdfasdfahsdfh_something() {}
  /// ```
  ///
  ///
  /// This `exported_symbols` vector will be:
  ///
  /// ```skip
  /// vec![
  ///     ExportedSymbol {
  ///         exported: "x",
  ///         local: "$abc$export$hashfashdfashdfahsdfh_x",
  ///         ...
  ///     },
  ///     ExportedSymbol {
  ///         exported: "something",
  ///         local: "$abc$export$hashfashdfashdfahsdfh_something",
  ///         ...
  ///     },
  /// ]
  /// ```
  pub exported_symbols: Vec<ExportedSymbol>,
  /// Symbols re-exported from other modules.
  ///
  /// If a symbol is re-exported from another module, parcel will remove the export statement
  /// from the asset.
  ///
  /// For example, if an input file is:
  ///
  /// ```skip
  /// export { view as mainView } from './view';
  /// ```
  ///
  /// The output will be
  /// ```skip
  /// import 'abc:./view:esm';
  /// ```
  ///
  /// And this vector will contain the information about the re-exported symbol.
  ///
  /// On this case, the fields of `ImportedSymbol` will mean different things than they do for
  /// [`HoistResult::imported_symbols`].
  ///
  /// In particular, since there is no mangled name, `local` means the "exported" name rather than
  /// the mangled name.
  ///
  /// On the case above, this field would be:
  ///
  /// ```skip
  /// vec![