ronantakizawa/codereview-bench huggingface dataset:
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/commands/types.ts (TypeScript)

Code before:
import { PackageJson, Reporter } from "gatsby"

export interface ICert {
  key: string
  cert: string
}

export interface IProgram {
  _: `develop` | `build` | `clean` | `feedback` | `repl` | `serve`
  command: `develop` | `build`
  status?: string // I think this type should not exist here. It seems to be added in the reducer, but not applicable to the caller site from gatsby-cli
  useYarn: boolean
  open: boolean
  openTracingConfigFile: string
  port: number
  proxyPort: number
  host: string
  report: Reporter
  [`cert-file`]?: string
  [`key-file`]?: string
  directory: string
  https?: boolean
  sitePackageJson: PackageJson
  ssl?: ICert
  graphqlTracing?: boolean
}

// @deprecated
export enum BuildHTMLStage {
  DevelopHTML = `develop-html`,
  BuildHTML = `build-html`,
}

export enum Stage {
  Develop = `develop`,

Review comment (suggestion):
I'd rather call it smth like "buildCommand", otherwise it's confusing that _ matches it partially. Also it should be nullable/optional for other IProgram users.

Code after:
import { PackageJson, Reporter } from "gatsby"

export interface ICert {
  key: string
  cert: string
}

export interface IProgram {
  _: `develop` | `build` | `clean` | `feedback` | `repl` | `serve`
  status?: string // I think this type should not exist here. It seems to be added in the reducer, but not applicable to the caller site from gatsby-cli
  useYarn: boolean
  open: boolean
  openTracingConfigFile: string
  port: number
  proxyPort: number
  host: string
  report: Reporter
  [`cert-file`]?: string
  [`key-file`]?: string
  directory: string
  https?: boolean
  sitePackageJson: PackageJson
  ssl?: ICert
  graphqlTracing?: boolean
}

// @deprecated
export enum BuildHTMLStage {
  DevelopHTML = `develop-html`,
  BuildHTML = `build-html`,
}

export enum Stage {
  Develop = `develop`,
  DevelopHTML = `develop-html`,
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/commands/develop-process.ts (TypeScript)

Code before:
          console.log(
            `${chalk.yellow(
              `warn`
            )} Browser not opened because no browser was found`
          )
        )
      }
    }

    isFirstCompile = false

    if (webpackActivity) {
      reportWebpackWarnings(stats)

      if (!isSuccessful) {
        const errors = structureWebpackErrors(
          `develop`,
          stats.compilation.errors
        )
        webpackActivity.panicOnBuild(errors)
      }
      webpackActivity.end()
      webpackActivity = null
    }

    await pageDataUtil.flush()
    webpackStatusUtil.markAsDone()

    done()
  })
}


Review comment (suggestion):
Considering we basically always flush before `markAsDone`, should it be part of `webpackStatusUtil`? Like a webpack transaction.

Code after:
              `warn`
            )} Browser not opened because no browser was found`
          )
        )
      }
    }

    isFirstCompile = false

    if (webpackActivity) {
      reportWebpackWarnings(stats)

      if (!isSuccessful) {
        const errors = structureWebpackErrors(
          `develop`,
          stats.compilation.errors
        )
        webpackActivity.panicOnBuild(errors)
      }
      webpackActivity.end()
      webpackActivity = null
    }

    enqueueFlush()
    markWebpackStatusAsDone()

    done()
  })
}
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/utils/page-data.ts (TypeScript)

Code before:
      const body = await writePageData(
        path.join(program.directory, `public`),
        page
      )

      if (program?._?.[0] === `develop`) {
        websocketManager.emitPageData({
          ...body.result,
          id: pagePath,
          result: {
            data: body.result.data,
            pageContext: body.result.pageContext,
          },
        })
      }
    }
  }

  store.dispatch({
    type: `CLEAR_PENDING_PAGE_DATA_WRITES`,
  })
  isFlushing = false
  return
}

export function enqueueFlush(): void {
  if (webpackStatus.isPending()) {
    isFlushPending = true
  } else {
    flush()
  }
}


Review comment (suggestion):
this name is misleading. it's not actually queuing. It's actually more of like a `flushIfReady` or something

Code after:
      const body = await writePageData(
        path.join(program.directory, `public`),
        page
      )

      if (program?._?.[0] === `develop`) {
        websocketManager.emitPageData({
          ...body.result,
          id: pagePath,
          result: {
            data: body.result.data,
            pageContext: body.result.pageContext,
          },
        })
      }
    }
  }

  store.dispatch({
    type: `CLEAR_PENDING_PAGE_DATA_WRITES`,
  })
  isFlushing = false
  return
}

export function enqueueFlush(): void {
  if (isWebpackStatusPending()) {
    isFlushPending = true
  } else {
    flush()
  }
}
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/utils/webpack-status.ts (TypeScript)

Code before:
import { flush, isFlushEnqueued } from "./page-data"

let isPendingStatus = false

export function isPending(): boolean {
  return isPendingStatus
}

export function markAsPending(): void {
  isPendingStatus = true
}

export function markAsDone(): void {
  isPendingStatus = false
  if (isFlushEnqueued()) {
    flush()
  }
}


Review comment (suggestion):
you could make this just a `flush` call and have it exit early if there isn't any flush enqueued

Code after:
import { flush, isFlushEnqueued } from "./page-data"

let isPendingStatus = false

export function isWebpackStatusPending(): boolean {
  return isPendingStatus
}

export function markWebpackStatusAsPending(): void {
  isPendingStatus = true
}

export function markWebpackStatusAsDone(): void {
  isPendingStatus = false
  if (isFlushEnqueued()) {
    flush()
  }
}
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/query/types.ts (TypeScript)

Code before:
  uniqueQueries: Set<string>
  totalRunQuery: number
  totalPluralRunQuery: number
  totalIndexHits: number
  totalSiftHits: number
  totalNonSingleFilters: number
  comparatorsUsed: Map<string, number>
  uniqueFilterPaths: Set<string>
  uniqueSorts: Set<string>
}

export interface IGraphQLRunnerStatResults {
  totalQueries: number
  uniqueOperations: number
  uniqueQueries: number
  totalRunQuery: number
  totalPluralRunQuery: number
  totalIndexHits: number
  totalSiftHits: number
  totalNonSingleFilters: number
  comparatorsUsed: Array<{ comparator: string; amount: number }>
  uniqueFilterPaths: number
  uniqueSorts: number
}

export type PageContext = unknown

export interface IExecutionResult extends ExecutionResult {
  pageContext?: PageContext
}


Review comment (suggestion):
This is a breaking change, because it means anything that uses context will need to narrow the type

Code after:
  uniqueQueries: Set<string>
  totalRunQuery: number
  totalPluralRunQuery: number
  totalIndexHits: number
  totalSiftHits: number
  totalNonSingleFilters: number
  comparatorsUsed: Map<string, number>
  uniqueFilterPaths: Set<string>
  uniqueSorts: Set<string>
}

export interface IGraphQLRunnerStatResults {
  totalQueries: number
  uniqueOperations: number
  uniqueQueries: number
  totalRunQuery: number
  totalPluralRunQuery: number
  totalIndexHits: number
  totalSiftHits: number
  totalNonSingleFilters: number
  comparatorsUsed: Array<{ comparator: string; amount: number }>
  uniqueFilterPaths: number
  uniqueSorts: number
}

export type PageContext = any

export interface IExecutionResult extends ExecutionResult {
  pageContext?: PageContext
}
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/utils/validate-page-component.ts (TypeScript)

Code before:

    if (fileContent === ``) {
      const relativePath = path.relative(directory, component)

      return {
        error: {
          id: `11327`,
          context: {
            relativePath,
          },
        },
        panicOnBuild: true,
      }
    }

    const includesDefaultExport =
      fileContent.includes(`export default`) ||
      fileContent.includes(`module.exports`) ||
      fileContent.includes(`exports.default`) ||
      fileContent.includes(`exports["default"]`) ||
      fileContent.match(/export \{.* as default.*\}/s) ||
      // this check only applies to js and ts, not mdx
      /\.(jsx?|tsx?)/.test(path.extname(component))

    if (!includesDefaultExport) {
      return {
        error: {
          id: `11328`,
          context: {
            component,
          },
        },
        panicOnBuild: true,
      }
    }
  }

  validationCache.add(component)
  return {}
}

export function clearValidationCache(): void {
  validationCache.clear()
}


Review comment (suggestion):
did it speedup? 
- when at first the `(jsx?|tsx?)/.test` is done 
- and then only if it match, then the calculation for `includesDefaultExport` with the many `manytext.includes` are calculated ?
- because if it is a mdx, then all 4 includes are calculated, because it is always not true for a mdx 

Code after:

    if (fileContent === ``) {
      const relativePath = path.relative(directory, component)

      return {
        error: {
          id: `11327`,
          context: {
            relativePath,
          },
        },
        panicOnBuild: true,
      }
    }

    const includesDefaultExport =
      fileContent.includes(`export default`) ||
      fileContent.includes(`module.exports`) ||
      fileContent.includes(`exports.default`) ||
      fileContent.includes(`exports["default"]`) ||
      fileContent.match(/export \{.* as default.*\}/s)

    // this check only applies to js and ts, not mdx
    if (
      /\.(jsx?|tsx?)/.test(path.extname(component)) &&
      !includesDefaultExport
    ) {
      return {
        error: {
          id: `11328`,
          context: {
            component,
          },
        },
        panicOnBuild: true,
      }
    }
  }

  validationCache.add(component)
  return {}
}

export function clearValidationCache(): void {
  validationCache.clear()
}
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/utils/validate-page-component.ts (TypeScript)

Code before:
  // Validate that the page component imports React and exports something
  // (hopefully a component).
  //

  if (
    !component.includes(`/.cache/`) &&
    process.env.NODE_ENV === `production`
  ) {
    const fileContent = fs.readFileSync(component, `utf-8`)

    if (fileContent === ``) {
      const relativePath = path.relative(directory, component)

      return {
        error: {
          id: `11327`,
          context: {
            relativePath,
          },
        },
        panicOnBuild: true,
      }
    }

    const includesDefaultExport =
      fileContent.includes(`export default`) ||
      fileContent.includes(`module.exports`) ||
      fileContent.includes(`exports.default`) ||
      fileContent.includes(`exports["default"]`) ||
      fileContent.match(/export \{.* as default.*\}/s)

    // this check only applies to js and ts, not mdx
    if (
      /\.(jsx?|tsx?)/.test(path.extname(component)) &&
      !includesDefaultExport
    ) {
      return {
        error: {
          id: `11328`,
          context: {
            component,
          },
        },
        panicOnBuild: true,
      }
    }
  }

  validationCache.add(component)
  return {}
}

Review comment (performance):
Can't we do this instead? I'm allergic to regex checks when efficient and simpler string comparisons suffice.

```suggestion
    const ext = path.extname(component);
    if (ext.endsWith('.jsx') || ext.endsWith('.tsx')) {
```
We can probably even drop the `path.extname()` call because we're only interested in the last bit.

Code after:
  // Validate that the page component imports React and exports something
  // (hopefully a component).
  //

  if (
    !component.includes(`/.cache/`) &&
    process.env.NODE_ENV === `production`
  ) {
    const fileContent = fs.readFileSync(component, `utf-8`)

    if (fileContent === ``) {
      const relativePath = path.relative(directory, component)

      return {
        error: {
          id: `11327`,
          context: {
            relativePath,
          },
        },
        panicOnBuild: true,
      }
    }

    // this check only applies to js and ts, not mdx
    if (/\.(jsx?|tsx?)/.test(path.extname(component))) {
      const includesDefaultExport =
        fileContent.includes(`export default`) ||
        fileContent.includes(`module.exports`) ||
        fileContent.includes(`exports.default`) ||
        fileContent.includes(`exports["default"]`) ||
        fileContent.match(/export \{.* as default.*\}/s)

      if (!includesDefaultExport) {
        return {
          error: {
            id: `11328`,
            context: {
              component,
            },
          },
          panicOnBuild: true,
        }
      }
    }
  }

  validationCache.add(component)
  return {}
}
Repository: gatsbyjs/gatsby
File: packages/gatsby-plugin-sharp/src/plugin-options.js (JavaScript)

Code before:
const _ = require(`lodash`)

/// Plugin options are loaded onPreBootstrap in gatsby-node
const pluginDefaults = {
  forceBase64Format: false,
  useMozJpeg: process.env.GATSBY_JPEG_ENCODER === `MOZJPEG`,
  stripMetadata: true,
  lazyImageGeneration: true,
  defaultQuality: 50,
}

const generalArgs = {
  quality: 50,
  jpegQuality: null,
  pngQuality: null,
  webpQuality: null,
  jpegProgressive: true,
  pngCompressionLevel: 9,
  // default is 4 (https://github.com/kornelski/pngquant/blob/4219956d5e080be7905b5581314d913d20896934/rust/bin.rs#L61)
  pngCompressionSpeed: 4,
  base64: true,
  grayscale: false,
  duotone: false,
  pathPrefix: ``,
  toFormat: ``,
  toFormatBase64: ``,
  sizeByPixelDensity: false,
  rotate: 0,
}

let pluginOptions = Object.assign({}, pluginDefaults)

Review comment (suggestion):
This got omitted somehow, I probably didn't click on start-review...
Can't we change this to undefined instead? Technically this becomes an enum with 3 values then. We could add validation later on.
```suggestion
```

Code after:
const _ = require(`lodash`)

/// Plugin options are loaded onPreBootstrap in gatsby-node
const pluginDefaults = {
  base64Width: 20,
  forceBase64Format: ``, // valid formats: png,jpg,webp
  useMozJpeg: process.env.GATSBY_JPEG_ENCODER === `MOZJPEG`,
  stripMetadata: true,
  lazyImageGeneration: true,
  defaultQuality: 50,
}

const generalArgs = {
  quality: 50,
  jpegQuality: null,
  pngQuality: null,
  webpQuality: null,
  jpegProgressive: true,
  pngCompressionLevel: 9,
  // default is 4 (https://github.com/kornelski/pngquant/blob/4219956d5e080be7905b5581314d913d20896934/rust/bin.rs#L61)
  pngCompressionSpeed: 4,
  base64: true,
  grayscale: false,
  duotone: false,
  pathPrefix: ``,
  toFormat: ``,
  toFormatBase64: ``,
  sizeByPixelDensity: false,
  rotate: 0,
}
Repository: gatsbyjs/gatsby
File: packages/gatsby-source-drupal/src/utils.js (JavaScript)

Code before:
    removedReferencedNodes.forEach(referencedNode => {
      referencedNode.relationships[
        nodeFieldName
      ] = referencedNode.relationships[nodeFieldName].filter(
        id => id !== newNode.id
      )
    })

    // see what nodes are newly referenced, and make sure to call `createNode` on them
    const addedReferencedNodes = _.difference(
      newNodeReferencedNodes,
      oldNodeReferencedNodes
    ).map(id => getNode(id))

    nodesToUpdate.push(...addedReferencedNodes)
  } else {
    // if we are inserting new node, we need to update all referenced nodes
    const newNodes = referencedNodesLookup.get(newNode)
    if (typeof newNodes !== `undefined`) {
      newNodes.forEach(id => nodesToUpdate.push(getNode(id)))
    }
  }

  // download file
  const { skipFileDownloads } = pluginOptions
  if (isFileNode(newNode) && skipFileDownloads !== false) {
    await downloadFile(
      {
        node: newNode,
        store,
        cache,
        createNode,
        createNodeId,
        getCache,
      },
      pluginOptions
    )
  }

  for (const node of nodesToUpdate) {
    if (node.internal.owner) {
      delete node.internal.owner
    }
    node.internal.contentDigest = createContentDigest(node)
    createNode(node)
    reporter.log(`Updated node: ${node.id}`)
  }
}

exports.handleWebhookUpdate = handleWebhookUpdate


Review comment (suggestion):
This looks suspicious. "If file node and skipFileDownloads is true, then download the file"?

Also, above it is is set to `false` if it's falsy. This case is a strict comparison. Seems to me like both usages should do the same thing.

Code after:
    removedReferencedNodes.forEach(referencedNode => {
      referencedNode.relationships[
        nodeFieldName
      ] = referencedNode.relationships[nodeFieldName].filter(
        id => id !== newNode.id
      )
    })

    // see what nodes are newly referenced, and make sure to call `createNode` on them
    const addedReferencedNodes = _.difference(
      newNodeReferencedNodes,
      oldNodeReferencedNodes
    ).map(id => getNode(id))

    nodesToUpdate.push(...addedReferencedNodes)
  } else {
    // if we are inserting new node, we need to update all referenced nodes
    const newNodes = referencedNodesLookup.get(newNode)
    if (typeof newNodes !== `undefined`) {
      newNodes.forEach(id => nodesToUpdate.push(getNode(id)))
    }
  }

  // download file
  const { skipFileDownloads } = pluginOptions
  if (isFileNode(newNode) && !skipFileDownloads) {
    await downloadFile(
      {
        node: newNode,
        store,
        cache,
        createNode,
        createNodeId,
        getCache,
      },
      pluginOptions
    )
  }

  for (const node of nodesToUpdate) {
    if (node.internal.owner) {
      delete node.internal.owner
    }
    node.internal.contentDigest = createContentDigest(node)
    createNode(node)
    reporter.log(`Updated node: ${node.id}`)
  }
}

exports.handleWebhookUpdate = handleWebhookUpdate
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/redux/run-fast-filters.ts (TypeScript)

Code before:
import { IGatsbyNode } from "./types"
import { GatsbyGraphQLType } from "../../index"
import { prepareRegex } from "../utils/prepare-regex"
const { makeRe } = require(`micromatch`)
import { getValueAt } from "../utils/get-value-at"
import _ from "lodash"
import {
  DbQuery,
  IDbQueryQuery,
  IDbQueryElemMatch,
  objectToDottedField,
  createDbQueriesFromObject,
  prefixResolvedFields,
} from "../db/common/query"
import {
  FilterOp,
  FilterCacheKey,
  FiltersCache,
  FilterValueNullable,
  ensureEmptyFilterCache,
  ensureIndexByQuery,
  ensureIndexByElemMatch,
  getNodesFromCacheByValue,
  intersectNodesByCounter,
  IFilterCache,
} from "./nodes"
import { IGraphQLRunnerStats } from "../query/types"

Review comment (suggestion):
```suggestion
import { GatsbyGraphQLType } from "../.."
```
Don't explicitly import from index

Code after:
import { IGatsbyNode } from "./types"
import { GatsbyGraphQLType } from "../.."
import { prepareRegex } from "../utils/prepare-regex"
const { makeRe } = require(`micromatch`)
import { getValueAt } from "../utils/get-value-at"
import _ from "lodash"
import {
  DbQuery,
  IDbQueryQuery,
  IDbQueryElemMatch,
  objectToDottedField,
  createDbQueriesFromObject,
  prefixResolvedFields,
} from "../db/common/query"
import {
  FilterOp,
  FilterCacheKey,
  FiltersCache,
  FilterValueNullable,
  ensureEmptyFilterCache,
  ensureIndexByQuery,
  ensureIndexByElemMatch,
  getNodesFromCacheByValue,
  intersectNodesByCounter,
  IFilterCache,
} from "./nodes"
import { IGraphQLRunnerStats } from "../query/types"
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/redux/run-fast-filters.ts (TypeScript)

Code before:
import _ from "lodash"
import {
  DbQuery,
  IDbQueryQuery,
  IDbQueryElemMatch,
  objectToDottedField,
  createDbQueriesFromObject,
  prefixResolvedFields,
} from "../db/common/query"
import {
  FilterOp,
  FilterCacheKey,
  FiltersCache,
  FilterValueNullable,
  ensureEmptyFilterCache,
  ensureIndexByQuery,
  ensureIndexByElemMatch,
  getNodesFromCacheByValue,
  intersectNodesByCounter,
  IFilterCache,
} from "./nodes"
import { IGraphQLRunnerStats } from "../query/types"

interface IInputQuery {
  [key: string]: FilterValueNullable | IInputQuery
}
interface IPreparedQueryArg {
  [key: string]: FilterValueNullable | IPreparedQueryArg
}

/**
 * Creates a key for one filterCache inside FiltersCache
 */
function createFilterCacheKey(
  typeNames: Array<string>,
  filter: DbQuery | null
): FilterCacheKey {
  // Note: while `elemMatch` is a special case, in the key it's just `elemMatch`
  // (This function is future proof for elemMatch support, won't receive it yet)
  let f = filter
  let comparator = ``
  const paths: Array<string> = []
  while (f) {
    paths.push(...f.path)
    if (f.type === `elemMatch`) {
      const q: IDbQueryElemMatch = f
      f = q.nestedQuery
      // Make distinction between filtering `a.elemMatch.b.eq` and `a.b.eq`
      // In practice this is unlikely to be an issue, but it might
      paths.push(`elemMatch`)
    } else {

Review comment (refactor):
Could you add a comment here explaining why you can't use `Record`, in case someone tries to refactor it

Code after:
import _ from "lodash"
import {
  DbQuery,
  IDbQueryQuery,
  IDbQueryElemMatch,
  objectToDottedField,
  createDbQueriesFromObject,
  prefixResolvedFields,
} from "../db/common/query"
import {
  FilterOp,
  FilterCacheKey,
  FiltersCache,
  FilterValueNullable,
  ensureEmptyFilterCache,
  ensureIndexByQuery,
  ensureIndexByElemMatch,
  getNodesFromCacheByValue,
  intersectNodesByCounter,
  IFilterCache,
} from "./nodes"
import { IGraphQLRunnerStats } from "../query/types"

// The value is an object with arbitrary keys that are either filter values or,
// recursively, an object with the same struct. Ie. `{a: {a: {a: 2}}}`
interface IInputQuery {
  [key: string]: FilterValueNullable | IInputQuery
}
// Similar to IInputQuery except the comparator leaf nodes will have their
// key prefixed with `$` and their value, in some cases, normalized.
interface IPreparedQueryArg {
  [key: string]: FilterValueNullable | IPreparedQueryArg
}

interface IRunFilterArg {
  gqlType: GatsbyGraphQLType
  queryArgs: {
    filter: Array<IInputQuery> | undefined
    sort:
      | { fields: Array<string>; order: Array<boolean | "asc" | "desc"> }
      | undefined
  }
  firstOnly: boolean
  resolvedFields: Record<string, any>
  nodeTypeNames: Array<string>
  filtersCache: FiltersCache
  stats: IGraphQLRunnerStats
}

/**
 * Creates a key for one filterCache inside FiltersCache