ronantakizawa/codereview-bench huggingface dataset:
Repository: gatsbyjs/gatsby
File: packages/gatsby-dev-cli/src/local-npm-registry/install-packages.js (JavaScript)

Code before:
      $ yarn workspaces info --json
      yarn workspaces v1.22.0
      {
        "z": {
          "location": "z",
          "workspaceDependencies": [],
          "mismatchedWorkspaceDependencies": []
        },
        "y": {
          "location": "y",
          "workspaceDependencies": [],
          "mismatchedWorkspaceDependencies": []
        }
      }
      Done in 0.48s.
      ```
      So we need to do some sanitization. We find JSON by matching substring
      that starts with `{` and ends with `}`
    */

      const regex = /^[^{]*({.*})[^}]*$/gs
      const sanitizedStdOut = regex.exec(stdout)
      if (sanitizedStdOut?.length >= 2) {
        // pick content of first (and only) capturing group
        const jsonString = sanitizedStdOut[1]
        workspacesLayout = JSON.parse(jsonString)
      }
    }

    if (!workspacesLayout) {
      console.log(
        `Couldn't parse output of "yarn workspaces info" command`,
        stdout
      )
      process.exit(1)
    }

    const handleDeps = deps => {
      if (!deps) {
        return false
      }

      let changed = false
      Object.keys(deps).forEach(depName => {
        if (packagesToInstall.includes(depName)) {
          deps[depName] = `gatsby-dev`
          changed = true
        }
      })
      return changed
    }

Review comment (suggestion):
Should probably try/catch this too, emit a sensible error (for ourselves).

Code after:
      $ yarn workspaces info --json
      yarn workspaces v1.22.0
      {
        "z": {
          "location": "z",
          "workspaceDependencies": [],
          "mismatchedWorkspaceDependencies": []
        },
        "y": {
          "location": "y",
          "workspaceDependencies": [],
          "mismatchedWorkspaceDependencies": []
        }
      }
      Done in 0.48s.
      ```
      So we need to do some sanitization. We find JSON by matching substring
      that starts with `{` and ends with `}`
    */

      const regex = /^[^{]*({.*})[^}]*$/gs
      const sanitizedStdOut = regex.exec(stdout)
      if (sanitizedStdOut?.length >= 2) {
        // pick content of first (and only) capturing group
        const jsonString = sanitizedStdOut[1]
        try {
          workspacesLayout = JSON.parse(jsonString)
        } catch (e) {
          console.error(
            `Failed to parse "sanitized" output of "yarn workspaces info" command.\n\nSanitized string: "${jsonString}`
          )
          // not exitting here, because we have general check for `workspacesLayout` being set below
        }
      }
    }

    if (!workspacesLayout) {
      console.error(
        `Couldn't parse output of "yarn workspaces info" command`,
        stdout
      )
      process.exit(1)
    }

    const handleDeps = deps => {
      if (!deps) {
        return false
      }

      let changed = false
      Object.keys(deps).forEach(depName => {
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/redux/run-sift.js (JavaScript)

Code before:
  if (!filtersCache) {
    // If no filter cache is passed on, explicitly don't use one
    return null
  }

  const nodesPerValueArrs /*: Array<Array<IGatsbyNode>> */ = getBucketsForFilters(
    filters,
    nodeTypeNames,
    filtersCache
  )

  if (!nodesPerValueArrs) {
    return null
  }

  if (nodesPerValueArrs.length === 0) {
    return []
  }

  // Put smallest last (we'll pop it)
  nodesPerValueArrs.sort(
    (a /*: Array<IGatsbyNode> */, b /*: Array<IGatsbyNode> */) =>
      b.length - a.length
  )

  if (nodesPerValueArrs.length === 1) {
    // If there's only one bucket then we have to run it against itself to
    // make sure it doesn't contain dupes. Otherwise the deduping would
    // already happen while merging.
    nodesPerValueArrs.push(nodesPerValueArrs[nodesPerValueArrs.length - 1])
  }

  while (nodesPerValueArrs.length > 1) {
    nodesPerValueArrs.push(
      intersectNodesByCounter(nodesPerValueArrs.pop(), nodesPerValueArrs.pop())
    )
  }

  const result = nodesPerValueArrs[0]

  if (result.length === 0) {
    // Intersection came up empty. Not one node appeared in every bucket.
    return null
  }

  return result
}

// Not a public API
exports.filterWithoutSift = filterWithoutSift


Review comment (performance):
Note: this is the main step I'm trying to prevent because this happens at "runtime" (n times) not "cache generation time" (one time), even for single filters. It's just better on all accounts :)

Code after:
  if (!filtersCache) {
    // If no filter cache is passed on, explicitly don't use one
    return null
  }

  const nodesPerValueArrs /*: Array<Array<IGatsbyNode>> */ = getBucketsForFilters(
    filters,
    nodeTypeNames,
    filtersCache
  )

  if (!nodesPerValueArrs) {
    return null
  }

  if (nodesPerValueArrs.length === 0) {
    return []
  }

  // Put smallest last (we'll pop it)
  nodesPerValueArrs.sort(
    (a /*: Array<IGatsbyNode> */, b /*: Array<IGatsbyNode> */) =>
      b.length - a.length
  )

  // All elements of nodesPerValueArrs should be sorted by counter and deduped
  // So if there's only one bucket in this list the next loop is skipped

  while (nodesPerValueArrs.length > 1) {
    nodesPerValueArrs.push(
      intersectNodesByCounter(nodesPerValueArrs.pop(), nodesPerValueArrs.pop())
    )
  }

  const result = nodesPerValueArrs[0]

  if (result.length === 0) {
    // Intersection came up empty. Not one node appeared in every bucket.
    return null
  }

  return result
}

// Not a public API
exports.filterWithoutSift = filterWithoutSift

/**
 * @param {Array<DbQuery>} filters
 * @param {Array<string>} nodeTypeNames
 * @param {FiltersCache} filtersCache
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/utils/validate-page-component.ts (TypeScript)

Code before:
import path from "path"
import fs from "fs-extra"
import { IGatsbyPage } from "../redux/types"

const validationCache = new Map<string, boolean>()

interface IErrorMeta {
  id: string
  context: Record<string, unknown>
}

export function validatePageComponent(
  page: IGatsbyPage,
  directory: string,
  pluginName: string
): { message?: string; error?: IErrorMeta; panicOnBuild?: boolean } {
  const { component } = page
  if (!component) {
    throw new Error(`11322`)
  }
  if (validationCache.has(component)) {
    return {}
  }
  if (!path.isAbsolute(component)) {
    return {
      error: {
        id: `11326`,
        context: {
          pluginName,
          pageObject: page,

Review comment (performance):
A set seems more appropriate.

```suggestion
const validationCache = new Set<string>()
```

Code after:
import path from "path"
import fs from "fs-extra"
import { IGatsbyPage } from "../redux/types"

const validationCache = new Set<string>()

interface IErrorMeta {
  id: string
  context: Record<string, unknown>
}

export function validatePageComponent(
  page: IGatsbyPage,
  directory: string,
  pluginName: string
): { message?: string; error?: IErrorMeta; panicOnBuild?: boolean } {
  const { component } = page
  if (!component) {
    throw new Error(`11322`)
  }
  if (validationCache.has(component)) {
    return {}
  }
  if (!path.isAbsolute(component)) {
    return {
      error: {
        id: `11326`,
        context: {
          pluginName,
          pageObject: page,
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/utils/validate-page-component.ts (TypeScript)

Code before:
      }
    }

    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.set(component, true)
  return {}
}

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


Review comment (performance):
```suggestion
  validationCache.add(component)
```

Code after:
      }
    }

    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()
}
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/commands/develop.ts (TypeScript)

Code before:
      })
    }

    return new Promise(resolve => {
      this.process.on(`exit`, () => {
        this.process.removeAllListeners()
        resolve()
      })
    })
  }
  on(type, callback): void {
    this.process.on(type, callback)
  }
}

let isRestarting

module.exports = async (program: IProgram): Promise<void> => {
  const developProcessPath = slash(require.resolve(`./develop-process`))
  // Run the actual develop server on a random port, and the proxy on the program port
  // which users will access
  const proxyPort = program.port
  const [statusServerPort, developPort, debugPort] = await Promise.all([
    getRandomPort(),
    getRandomPort(),
    program.inspectBrk ? getRandomPort() : Promise.resolve(null),
  ])

  const developProcess = new ControllableScript(
    `
    const cmd = require(${JSON.stringify(developProcessPath)});
    const args = ${JSON.stringify({
      ...program,
      port: developPort,
      proxyPort,
      debugPort,
    })};
    cmd(args);
  `,
    debugPort
  )

  const proxy = startDevelopProxy({
    proxyPort: proxyPort,
    targetPort: developPort,
    programPath: program.directory,
  })

  let unlock
  if (!isCI()) {
    unlock = await createServiceLock(program.directory, `developstatusserver`, {

Review comment (suggestion):
When using `node --inspect(-brk)` port by default is 9229 (not random). 

It would also be great if we could follow `node --inspect-brk` semantics - which allow us to specify port `--inspect-brk=9230` or skip specyfying port and then fallback to default

Code after:
    this.isRunning = true
    const args = [tmpFileName]
    // Passing --inspect isn't necessary for the child process to launch a port but it allows some editors to automatically attach
    if (this.debugInfo) {
      args.push(
        this.debugInfo.break
          ? `--inspect-brk=${this.debugInfo.port}`
          : `--inspect=${this.debugInfo.port}`
      )
    }

    this.process = spawn(`node`, args, {
      env: process.env,
      stdio: [`inherit`, `inherit`, `inherit`, `ipc`],
    })
  }
  async stop(signal: string | null = null, code?: number): Promise<void> {
    this.isRunning = false
    if (signal) {
      this.process.kill(signal)
    } else {
      this.process.send({
        type: `COMMAND`,
        action: {
          type: `EXIT`,
          payload: code,
        },
      })
    }

    return new Promise(resolve => {
      this.process.on(`exit`, () => {
        this.process.removeAllListeners()
        resolve()
      })
    })
  }
  on(type, callback): void {
    this.process.on(type, callback)
  }
}

let isRestarting

// checks if a string is a valid ip
const REGEX_IP = /^(?:(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9]?[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9]?[0-9])$/

module.exports = async (program: IProgram): Promise<void> => {
  const developProcessPath = slash(require.resolve(`./develop-process`))
  // Run the actual develop server on a random port, and the proxy on the program port
  // which users will access
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/services/calculate-dirty-queries.ts (TypeScript)

Code before:
import { calcInitialDirtyQueryIds, groupQueryIds } from "../query"
import { IBuildContext } from "./"

export interface IGroupedQueryIds {
  pageQueryIds: string[]
  staticQueryIds: string[]
}

export async function calculateDirtyQueries({
  store,
  filesDirty,
}: Partial<IBuildContext>): Promise<{
  queryIds: IGroupedQueryIds
}> {
  const state = store?.getState()
  if (filesDirty) {
    // Do stuff
  }

  const queryIds = calcInitialDirtyQueryIds(state)
  return { queryIds: groupQueryIds(queryIds) }
}


Review comment (refactor):
Let's remove this from this PR and re-add when we have more things in place when we actually can "do stuff" here ;)

Code after:
import { calcInitialDirtyQueryIds, groupQueryIds } from "../query"
import { IBuildContext, IGroupedQueryIds } from "./"
import reporter from "gatsby-cli/lib/reporter"

export async function calculateDirtyQueries({
  store,
}: Partial<IBuildContext>): Promise<{
  queryIds: IGroupedQueryIds
}> {
  if (!store) {
    reporter.panic(`Cannot run service without a redux store`)
  }
  const state = store.getState()
  // TODO: Check filesDirty from context

  const queryIds = calcInitialDirtyQueryIds(state)
  return { queryIds: groupQueryIds(queryIds) }
}
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/commands/build.ts (TypeScript)

Code before:
  const publicDir = path.join(program.directory, `public`)
  initTracer(program.openTracingConfigFile)
  const buildActivity = report.phantomActivity(`build`)
  buildActivity.start()

  telemetry.trackCli(`BUILD_START`)
  signalExit(exitCode => {
    telemetry.trackCli(`BUILD_END`, { exitCode })
  })

  const buildSpan = buildActivity.span
  buildSpan.setTag(`directory`, program.directory)

  const { graphqlRunner: bootstrapGraphQLRunner } = await bootstrap({
    ...program,
    parentSpan: buildSpan,
  })

  const graphqlRunner = new GraphQLRunner(store, {
    collectStats: true,
    graphqlTracing: program.graphqlTracing,
  })

  const { queryIds } = await calculateDirtyQueries({ store })

  await runStaticQueries({ queryIds, parentSpan: buildSpan, store })

  await apiRunnerNode(`onPreBuild`, {
    graphql: bootstrapGraphQLRunner,
    parentSpan: buildSpan,
  })

  // Copy files from the static directory to
  // an equivalent static directory within public.
  copyStaticDirs()

  const buildActivityTimer = report.activityTimer(
    `Building production JavaScript and CSS bundles`,
    { parentSpan: buildSpan }
  )
  buildActivityTimer.start()
  let stats
  try {
    stats = await buildProductionBundle(program, buildActivityTimer.span)
  } catch (err) {
    buildActivityTimer.panic(structureWebpackErrors(Stage.BuildJavascript, err))
  } finally {
    buildActivityTimer.end()
  }

  const workerPool = WorkerPool.create()

Review comment (suggestion):
We probably would want to pass `graphqlRunner` instance here (and in `runPageQueries`) to avoid lazily creating multiple ones

Code after:
  const publicDir = path.join(program.directory, `public`)
  initTracer(program.openTracingConfigFile)
  const buildActivity = report.phantomActivity(`build`)
  buildActivity.start()

  telemetry.trackCli(`BUILD_START`)
  signalExit(exitCode => {
    telemetry.trackCli(`BUILD_END`, { exitCode })
  })

  const buildSpan = buildActivity.span
  buildSpan.setTag(`directory`, program.directory)

  const { graphqlRunner: bootstrapGraphQLRunner } = await bootstrap({
    ...program,
    parentSpan: buildSpan,
  })

  const graphqlRunner = new GraphQLRunner(store, {
    collectStats: true,
    graphqlTracing: program.graphqlTracing,
  })

  const { queryIds } = await calculateDirtyQueries({ store })

  await runStaticQueries({
    queryIds,
    parentSpan: buildSpan,
    store,
    graphqlRunner,
  })

  await apiRunnerNode(`onPreBuild`, {
    graphql: bootstrapGraphQLRunner,
    parentSpan: buildSpan,
  })

  // Copy files from the static directory to
  // an equivalent static directory within public.
  copyStaticDirs()

  const buildActivityTimer = report.activityTimer(
    `Building production JavaScript and CSS bundles`,
    { parentSpan: buildSpan }
  )
  buildActivityTimer.start()
  let stats
  try {
    stats = await buildProductionBundle(program, buildActivityTimer.span)
  } catch (err) {
    buildActivityTimer.panic(structureWebpackErrors(Stage.BuildJavascript, err))
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/services/calculate-dirty-queries.ts (TypeScript)

Code before:
import { calcInitialDirtyQueryIds, groupQueryIds } from "../query"
import { IBuildContext } from "./"

export interface IGroupedQueryIds {
  pageQueryIds: string[]
  staticQueryIds: string[]
}

export async function calculateDirtyQueries({
  store,
}: Partial<IBuildContext>): Promise<{
  queryIds: IGroupedQueryIds
}> {
  const state = store?.getState()
  // TODO: Check filesDirty from context

  const queryIds = calcInitialDirtyQueryIds(state)
  return { queryIds: groupQueryIds(queryIds) }
}


Review comment (bug):
Does it make sense to do a `?.` here? store will never be nullable. I Feel like typings are wrong

Code after:
import { calcInitialDirtyQueryIds, groupQueryIds } from "../query"
import { IBuildContext, IGroupedQueryIds } from "./"
import reporter from "gatsby-cli/lib/reporter"

export async function calculateDirtyQueries({
  store,
}: Partial<IBuildContext>): Promise<{
  queryIds: IGroupedQueryIds
}> {
  if (!store) {
    reporter.panic(`Cannot run service without a redux store`)
  }
  const state = store.getState()
  // TODO: Check filesDirty from context

  const queryIds = calcInitialDirtyQueryIds(state)
  return { queryIds: groupQueryIds(queryIds) }
}
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/services/types.ts (TypeScript)

Code before:
import { WebsocketManager } from "../utils/websocket-manager"
import { AnyEventObject, Actor } from "xstate"
import { Compiler } from "webpack"
import { Span } from "opentracing"
import { Express } from "express"
import JestWorker from "jest-worker"
import { IProgram } from "../commands/types"
import { Store } from "../.."
import { GraphQLRunner } from "../query/graphql-runner"

export interface IGroupedQueryIds {
  pageQueryIds: string[]
  staticQueryIds: string[]
}

export interface IMutationAction {
  type: string
  payload: unknown[]
}
export interface IBuildContext {
  program?: IProgram
  app?: Express
  recursionCount: number
  nodesMutatedDuringQueryRun: boolean
  firstRun: boolean
  nodeMutationBatch: IMutationAction[]
  filesDirty?: boolean
  runningBatch: IMutationAction[]
  compiler?: Compiler
  websocketManager?: WebsocketManager
  store?: Store
  parentSpan?: Span
  graphqlRunner?: GraphQLRunner
  refresh?: boolean
  webhookBody?: Record<string, unknown>
  queryIds?: IGroupedQueryIds
  workerPool?: JestWorker
  pagesToBuild?: string[]
  pagesToDelete?: string[]
  mutationListener?: Actor<any, AnyEventObject>
}


Review comment (suggestion):
I don't think we need firstRun here
```suggestion
```

Code after:
import { Span } from "opentracing"
import { IProgram } from "../commands/types"
import { Store } from "../.."
import { GraphQLRunner } from "../query/graphql-runner"

export interface IGroupedQueryIds {
  pageQueryIds: string[]
  staticQueryIds: string[]
}

export interface IMutationAction {
  type: string
  payload: unknown[]
}
export interface IBuildContext {
  program?: IProgram
  store?: Store
  parentSpan?: Span
  graphqlRunner?: GraphQLRunner
  queryIds?: IGroupedQueryIds
}
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/services/run-page-queries.ts (TypeScript)

Code before:
import { processPageQueries } from "../query"
import { IBuildContext } from "./"
import reporter from "gatsby-cli/lib/reporter"

export async function runPageQueries({
  parentSpan,
  queryIds,
  store,
  program,
  graphqlRunner,
}: Partial<IBuildContext>): Promise<void> {
  if (!queryIds || !store) {
    return
  }
  const { pageQueryIds } = queryIds
  const state = store.getState()
  const pageQueryIdsCount = pageQueryIds.filter(id => state.pages.has(id))
    .length

  if (!pageQueryIdsCount) {
    return
  }

  const activity = reporter.createProgress(
    `run page queries`,
    pageQueryIdsCount,
    0,
    {
      id: `page-query-running`,
      parentSpan,
    }
  )

  activity.start()
  await processPageQueries(pageQueryIds, {
    state,
    activity,

Review comment (bug):
I feel typings are wrong here, these shouldn't be nullable

Code after:
import { processPageQueries } from "../query"
import { IBuildContext } from "./"
import reporter from "gatsby-cli/lib/reporter"

export async function runPageQueries({
  parentSpan,
  queryIds,
  store,
  program,
  graphqlRunner,
}: Partial<IBuildContext>): Promise<void> {
  if (!store) {
    reporter.panic(`Cannot run service without a redux store`)
  }
  if (!queryIds) {
    return
  }
  const { pageQueryIds } = queryIds
  const state = store.getState()
  const pageQueryIdsCount = pageQueryIds.filter(id => state.pages.has(id))
    .length

  if (!pageQueryIdsCount) {
    return
  }

  const activity = reporter.createProgress(
    `run page queries`,
    pageQueryIdsCount,
    0,
    {
      id: `page-query-running`,
      parentSpan,
    }
  )

  activity.start()
Repository: gatsbyjs/gatsby
File: packages/gatsby-image/index.d.ts (TypeScript)

Code before:
  className?: string | object
  critical?: boolean
  crossOrigin?: string | boolean
  style?: object
  imgStyle?: object
  placeholderStyle?: object
  placeholderClassName?: string
  backgroundColor?: string | boolean
  onLoad?: () => void
  onError?: (event: any) => void
  onStartLoad?: (param: { wasCached: boolean }) => void
  Tag?: string
  itemProp?: string
  loading?: `auto` | `lazy` | `eager`
  draggable?: boolean
}
  
interface GatsbyImageFluidProps extends GatsbyImageOptionalProps {
  fluid: FluidObject | FluidObject[]
}

interface GatsbyImageFixedProps extends GatsbyImageOptionalProps {
  fixed: FixedObject | FixedObject[]
}

interface GatsbyImageProps = GatsbyImageFluidProps | GatsbyImageFixedProps

export default class GatsbyImage extends React.Component<
  GatsbyImageProps,
  any
> {}


Review comment (suggestion):
```suggestion
type GatsbyImageProps = GatsbyImageFluidProps | GatsbyImageFixedProps
```

Code after:
   */
  sizes?: FluidObject
  fadeIn?: boolean
  durationFadeIn?: number
  title?: string
  alt?: string
  className?: string | object
  critical?: boolean
  crossOrigin?: string | boolean
  style?: object
  imgStyle?: object
  placeholderStyle?: object
  placeholderClassName?: string
  backgroundColor?: string | boolean
  onLoad?: () => void
  onError?: (event: any) => void
  onStartLoad?: (param: { wasCached: boolean }) => void
  Tag?: string
  itemProp?: string
  loading?: `auto` | `lazy` | `eager`
  draggable?: boolean
}
  
interface GatsbyImageFluidProps extends GatsbyImageOptionalProps {
  fluid: FluidObject | FluidObject[]
}

interface GatsbyImageFixedProps extends GatsbyImageOptionalProps {
  fixed: FixedObject | FixedObject[]
}

export type GatsbyImageProps = GatsbyImageFluidProps | GatsbyImageFixedProps

export default class GatsbyImage extends React.Component<
  GatsbyImageProps,
  any
> {}