ronantakizawa/codereview-bench huggingface dataset:
Repository: gatsbyjs/gatsby
File: packages/gatsby-source-filesystem/src/create-file-node.js (JavaScript)

Code before:
  stats.birthtime = stats.birthtime.toJSON()

  return {
    // Don't actually make the File id the absolute path as otherwise
    // people will use the id for that and ids shouldn't be treated as
    // useful information.
    id: createNodeId(pathToFile),
    children: [],
    parent: null,
    internal,
    sourceInstanceName: pluginOptions.name || `__PROGRAMMATIC__`,
    relativePath: slash(
      path.relative(
        pluginOptions.path || process.cwd(),
        slashedFile.absolutePath
      )
    ),
    extension: slashedFile.ext.slice(1).toLowerCase(),
    prettySize: prettyBytes(stats.size),
    modifiedTime: stats.mtime,
    accessTime: stats.atime,
    changeTime: stats.ctime,
    birthTime: stats.birthtime,
    ...slashedFile,
    ...stats,
  }
}


Review comment (suggestion):
can't we do this? I know nitpicks
```suggestion
  return {
    // Don't actually make the File id the absolute path as otherwise
    // people will use the id for that and ids shouldn't be treated as
    // useful information.
    id: createNodeId(pathToFile),
    children: [],
    parent: null,
    internal,
    sourceInstanceName: pluginOptions.name || `__PROGRAMMATIC__`,
    relativePath: slash(
      path.relative(
        pluginOptions.path || process.cwd(),
        slashedFile.absolutePath
      )
    ),
    extension: slashedFile.ext.slice(1).toLowerCase(),
    prettySize: prettyBytes(stats.size),
    ...stats,
    modifiedTime: stats.mtime.toJSON(),
    accessTime: stats.atime.toJSON(),
    changeTime: stats.ctime.toJSON(),
    birthTime: stats.birthtime.toJSON(),
    ...slashedFile,
  }
```

Code after:
    // useful information.
    id: createNodeId(pathToFile),
    children: [],
    parent: null,
    internal,
    sourceInstanceName: pluginOptions.name || `__PROGRAMMATIC__`,
    relativePath: slash(
      path.relative(
        pluginOptions.path || process.cwd(),
        slashedFile.absolutePath
      )
    ),
    extension: slashedFile.ext.slice(1).toLowerCase(),
    prettySize: prettyBytes(stats.size),
    modifiedTime: stats.mtime.toJSON(),
    accessTime: stats.atime.toJSON(),
    changeTime: stats.ctime.toJSON(),
    birthTime: stats.birthtime.toJSON(),
    // Note: deprecate splatting the slashedFile object
    // Note: the object may contain different properties depending on File or Dir
    ...slashedFile,
    // TODO: deprecate copying the entire object
    // Note: not splatting for perf reasons (make sure Date objects are serialized)
    dev: stats.dev,
    mode: stats.mode,
    nlink: stats.nlink,
    uid: stats.uid,
    rdev: stats.rdev,
    blksize: stats.blksize,
    ino: stats.ino,
    size: stats.size,
    blocks: stats.blocks,
    atimeMs: stats.atimeMs,
    mtimeMs: stats.mtimeMs,
    ctimeMs: stats.ctimeMs,
    birthtimeMs: stats.birthtimeMs,
    atime: stats.atime.toJSON(),
    mtime: stats.mtime.toJSON(),
    ctime: stats.ctime.toJSON(),
    birthtime: stats.birthtime.toJSON(),
  }
}
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/bootstrap/load-plugins/index.ts (TypeScript)

Code before:
    }
  }

  plugins.forEach(plugin => {
    flattened.push(plugin)
    extractPlugins(plugin)
  })

  return flattened
}

const normalizePlugins = (config: IRawSiteConfig = {}): ISiteConfig => {
  return {
    ...config,
    plugins: (config.plugins || []).map(
      (plugin): IPluginRefObject => {
        if (typeof plugin === `string`)
          return {
            resolve: plugin,
            options: {},
          }

        return plugin
      }
    ),
  }
}

export async function loadPlugins(
  rawConfig: IRawSiteConfig = {},
  rootDir: string | null = null
): Promise<Array<IFlattenedPlugin>> {
  // Turn all strings in plugins: [`...`] into the { resolve: ``, options: {} } form
  const config = normalizePlugins(rawConfig)

  // Show errors for invalid plugin configuration
  if (process.env.GATSBY_EXPERIMENTAL_PLUGIN_OPTION_VALIDATION) {
    await validatePluginOptions(config)
  }

  const currentAPIs = getAPI({
    browser: browserAPIs,
    node: nodeAPIs,
    ssr: ssrAPIs,
  })

  // Collate internal plugins, site config plugins, site default plugins
  const pluginInfos = loadPluginsInternal(config, rootDir)

  // Create a flattened array of the plugins
  const pluginArray = flattenPlugins(pluginInfos)

Review comment (suggestion):
Also normalize subplugins

```suggestion
  return {
    ...config,
    plugins: (config.plugins || []).map(
      (plugin): IPluginRefObject => {
        if (typeof plugin === `string`) {
          return {
            resolve: plugin,
            options: {},
          }
        }

        if (plugin.options.plugins) {
          plugin.options = normalizePlugins(plugin.options)
        }

        return plugin
      }
    ),
  }
```

Code after:
    }
  }

  plugins.forEach(plugin => {
    flattened.push(plugin)
    extractPlugins(plugin)
  })

  return flattened
}

function normalizePlugin(plugin): IPluginRefObject {
  if (typeof plugin === `string`) {
    return {
      resolve: plugin,
      options: {},
    }
  }

  if (plugin.options?.plugins) {
    plugin.options = {
      ...plugin.options,
      plugins: normalizePlugins(plugin.options.plugins),
    }
  }

  return plugin
}

function normalizePlugins(plugins?: Array<PluginRef>): Array<IPluginRefObject> {
  return (plugins || []).map(normalizePlugin)
}

const normalizeConfig = (config: IRawSiteConfig = {}): ISiteConfig => {
  return {
    ...config,
    plugins: (config.plugins || []).map(normalizePlugin),
  }
}

export async function loadPlugins(
  rawConfig: IRawSiteConfig = {},
  rootDir: string | null = null
): Promise<Array<IFlattenedPlugin>> {
  // Turn all strings in plugins: [`...`] into the { resolve: ``, options: {} } form
  const config = normalizeConfig(rawConfig)

  // Show errors for invalid plugin configuration
  if (process.env.GATSBY_EXPERIMENTAL_PLUGIN_OPTION_VALIDATION) {
    await validateConfigPluginsOptions(config)
  }
Repository: gatsbyjs/gatsby
File: packages/gatsby-source-contentful/src/fetch.js (JavaScript)

Code before:
const contentful = require(`contentful`)
const _ = require(`lodash`)
const chalk = require(`chalk`)
const { formatPluginOptionsForCLI } = require(`./plugin-options`)
const { CODES } = require(`./report`)

module.exports = async function contentfulFetch({
  syncToken,
  pluginConfig,
  reporter,
}) {
  // Fetch articles.
  let syncProgress
  const pageLimit = pluginConfig.get(`pageLimit`)
  let syncItemCount = 0
  const contentfulClientOptions = {
    space: pluginConfig.get(`spaceId`),
    accessToken: pluginConfig.get(`accessToken`),
    host: pluginConfig.get(`host`),
    environment: pluginConfig.get(`environment`),
    proxy: pluginConfig.get(`proxy`),
    responseLogger: response => {
      function createMetadataLog(response) {
        return [
          response?.headers[`content-length`] &&
            `size: ${response.headers[`content-length`]}B`,
          response?.headers[`x-contentful-request-id`] &&
            `request id: ${response.headers[`x-contentful-request-id`]}`,
          response?.headers[`x-cache`] &&
            `cache: ${response.headers[`x-cache`]}`,
        ]
          .filter(Boolean)
          .join(` `)
      }

      // Sync progress
      if (response.config.url === `sync`) {
        syncItemCount += response.data.items.length
        syncProgress.total = syncItemCount
        syncProgress.tick(response.data.items.length)

Review comment (suggestion):
nit, putting const and let together

```suggestion
  const pageLimit = pluginConfig.get(`pageLimit`)
  let syncProgress
  let syncItemCount = 0
```

Code after:
const contentful = require(`contentful`)
const _ = require(`lodash`)
const chalk = require(`chalk`)
const { formatPluginOptionsForCLI } = require(`./plugin-options`)
const { CODES } = require(`./report`)

module.exports = async function contentfulFetch({
  syncToken,
  pluginConfig,
  reporter,
}) {
  // Fetch articles.
  let syncProgress
  let syncItemCount = 0
  const pageLimit = pluginConfig.get(`pageLimit`)
  const contentfulClientOptions = {
    space: pluginConfig.get(`spaceId`),
    accessToken: pluginConfig.get(`accessToken`),
    host: pluginConfig.get(`host`),
    environment: pluginConfig.get(`environment`),
    proxy: pluginConfig.get(`proxy`),
    responseLogger: response => {
      function createMetadataLog(response) {
        if (process.env.gatsby_log_level === `verbose`) {
          return ``
        }
        return [
          response?.headers[`content-length`] &&
            `size: ${response.headers[`content-length`]}B`,
          response?.headers[`x-contentful-request-id`] &&
            `request id: ${response.headers[`x-contentful-request-id`]}`,
          response?.headers[`x-cache`] &&
            `cache: ${response.headers[`x-cache`]}`,
        ]
          .filter(Boolean)
          .join(` `)
      }

      // Sync progress
      if (response.config.url === `sync`) {
Repository: gatsbyjs/gatsby
File: packages/gatsby-telemetry/src/get-dependencies.ts (TypeScript)

Code before:
import { readFileSync } from "fs"

export function getDependencies(): {
  dependencies: Array<string> | undefined
  devDependencies: Array<string> | undefined
} {
  const data = parsePackageJson()

  return {
    dependencies: mapData(data?.dependencies),
    devDependencies: mapData(data?.devDependencies),
  }
}

function mapData(deps: object): Array<string> | undefined {
  if (!deps) {
    return undefined
  }
  return Object.entries(deps)
    .map(([name, version]) => {
      return {
        name,
        version,
      }
    })
    .map(({ name, version }) => `${name}@${version}`)

Review comment (suggestion):
we use fs-extra for better os support
```suggestion
import { readFileSync } from "fs-extra"
```

Code after:
import { readFileSync } from "fs-extra"

export function getDependencies(): {
  dependencies: Array<string> | undefined
  devDependencies: Array<string> | undefined
} {
  const data = parsePackageJson()

  return {
    dependencies: mapData(data?.dependencies),
    devDependencies: mapData(data?.devDependencies),
  }
}

function mapData(deps: object): Array<string> | undefined {
  if (!deps) {
    return undefined
  }
  return Object.entries(deps)
    .map(([name, version]) => {
      return {
        name,
        version,
      }
    })
    .map(({ name, version }) => `${name}@${version}`)
Repository: gatsbyjs/gatsby
File: packages/gatsby-plugin-page-creator/src/__tests__/extract-query.ts (TypeScript)

Code before:
      expect(
        generateQueryFromString(
          `_type123`,
          compatiblePath(`/foo/{_type123._field123}.js`)
        )
      ).toBe(`{allType123{nodes{_field123,id}}}`)
    })

    it(`works with different file extensions`, () => {
      expect(
        generateQueryFromString(
          `Thing`,
          compatiblePath(`/foo/bar/{Thing.id}.tsx`)
        )
      ).toBe(`{allThing{nodes{id}}}`)
    })

    it(`works with arguments`, () => {
      expect(
        generateQueryFromString(
          `{ allThing(filter: { main_url: { nin: [] }}) { ...CollectionPagesQueryFragment } }`,
          compatiblePath(`/foo/bar/{Thing.id}.js`)
        )
      ).toBe(`{ allThing(filter: { main_url: { nin: [] }}) { nodes{id} } }`)
    })
  })

  describe(`filepath resolution`, () => {
    it(`basic example`, () => {
      expect(
        generateQueryFromString(
          `Thing`,
          compatiblePath(`/foo/bar/{Thing.id}.js`)
        )
      ).toBe(`{allThing{nodes{id}}}`)
    })

    it(`always queries id`, () => {
      expect(
        generateQueryFromString(
          `Thing`,
          compatiblePath(`/foo/bar/{Thing.baz}.js`)
        )
      ).toBe(`{allThing{nodes{baz,id}}}`)
    })

    it(`multiple nodes`, () => {
      expect(
        generateQueryFromString(
          `Thing`,
          compatiblePath(`/foo/bar/{Thing.id}/{Thing.name}.js`)

Review comment (suggestion):
Forgot to remove that in previous PR (about `collectionGraphql`)

Code after:
      expect(
        generateQueryFromString(
          `_type123`,
          compatiblePath(`/foo/{_type123._field123}.js`)
        )
      ).toBe(`{allType123{nodes{_field123,id}}}`)
    })

    it(`works with different file extensions`, () => {
      expect(
        generateQueryFromString(
          `Thing`,
          compatiblePath(`/foo/bar/{Thing.id}.tsx`)
        )
      ).toBe(`{allThing{nodes{id}}}`)
    })
  })

  describe(`filepath resolution`, () => {
    it(`basic example`, () => {
      expect(
        generateQueryFromString(
          `Thing`,
          compatiblePath(`/foo/bar/{Thing.id}.js`)
        )
      ).toBe(`{allThing{nodes{id}}}`)
    })

    it(`always queries id`, () => {
      expect(
        generateQueryFromString(
          `Thing`,
          compatiblePath(`/foo/bar/{Thing.baz}.js`)
        )
      ).toBe(`{allThing{nodes{baz,id}}}`)
    })

    it(`multiple nodes`, () => {
      expect(
        generateQueryFromString(
          `Thing`,
          compatiblePath(`/foo/bar/{Thing.id}/{Thing.name}.js`)
        )
      ).toBe(`{allThing{nodes{id,name}}}`)
      expect(
        generateQueryFromString(
          `Thing`,
          compatiblePath(`/foo/bar/{Thing.id}-{Thing.name}.js`)
        )
      ).toBe(`{allThing{nodes{id,name}}}`)
    })
Repository: gatsbyjs/gatsby
File: packages/gatsby-plugin-page-creator/src/__tests__/validate-path-query.ts (TypeScript)

Code before:
`)
  })

  it(`throws error if the file does not exist`, () => {
    expect(() => {
      validatePathQuery(`/foo/{bar}`, [`.js`, `.ts`, `.mjs`])
    }).toThrowErrorMatchingInlineSnapshot(`
"PageCreator: To query node \\"gatsbyPath\\" the \\"filePath\\" argument must represent a file that exists.
Unable to find a file at: \\"<PROJECT_ROOT>/src/pages/foo/{bar}\\""
`)
  })

  it(`works if all the above predicates pass`, async () => {
    const filePath = `/foo/{bar}-{baz}_{a}.{b}`
    const absolutePath = systemPath.join(
      process.cwd(),
      `src/pages`,
      `${filePath}.js`
    )
    await fs.createFile(absolutePath)

    expect(() => {
      validatePathQuery(filePath, [`.js`, `.ts`, `.mjs`])
    }).not.toThrow()

    await fs.remove(systemPath.join(process.cwd(), `src/pages`))
  })
})


Review comment (suggestion):
Running this locally would remove all source files for the plugin itself otherwise 🤣 
happened once... thanks IntelliJ history!

Code after:
`)
  })

  it(`throws error if the file does not exist`, () => {
    expect(() => {
      validatePathQuery(`/foo/{bar}`, [`.js`, `.ts`, `.mjs`])
    }).toThrowErrorMatchingInlineSnapshot(`
"PageCreator: To query node \\"gatsbyPath\\" the \\"filePath\\" argument must represent a file that exists.
Unable to find a file at: \\"<PROJECT_ROOT>/src/pages/foo/{bar}\\""
`)
  })

  it(`works if all the above predicates pass`, async () => {
    const filePath = `/baz/{bar}`
    const absolutePath = systemPath.join(
      process.cwd(),
      `src/pages`,
      `${filePath}.js`
    )
    await fs.createFile(absolutePath)

    expect(() => {
      validatePathQuery(filePath, [`.js`, `.ts`, `.mjs`])
    }).not.toThrow()

    await fs.remove(systemPath.join(process.cwd(), `src/pages`))
  })
})
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/query/graphql-runner.ts (TypeScript)

Code before:

      this.stats.uniqueQueries.add(
        crypto.createHash(`sha1`).update(statsQuery).digest(`hex`)
      )
    }

    const document = this.parse(query)
    const errors = this.validate(schema, document)

    let tracer
    if (this.graphqlTracing && parentSpan) {
      tracer = new GraphQLSpanTracer(`GraphQL Query`, {
        parentSpan,
        tags: {
          queryName: queryName,
        },
      })

      tracer.start()
    }

    try {
      const result =
        errors.length > 0
          ? { errors }
          : await execute({
              schema,
              document,
              rootValue: context,
              contextValue: withResolverContext({
                schema,
                schemaComposer: schemaCustomization.composer,
                context,
                customContext: schemaCustomization.context,
                nodeModel: this.nodeModel,
                stats: this.stats,
                tracer,
              }),
              variableValues: context,
            })

      // Queries are usually executed in batch. But after the batch is finished
      // cache just wastes memory without much benefits.
      // TODO: consider a better strategy for cache purging/invalidation
      this.scheduleClearCache()

      return result
    } finally {
      if (tracer) {
        tracer.end()
      }

Review comment (performance):
This will change the dynamics of query execution. Before this change, the `scheduleClearCache` function would've been called immediately after the `execute` call. Now it waits for `execute` to finish.

I have no idea how it will actually impact query running in real projects. Just want to bring your attention as this is not just a syntax change and definitely not a `chore`.

Code after:

      this.stats.uniqueQueries.add(
        crypto.createHash(`sha1`).update(statsQuery).digest(`hex`)
      )
    }

    const document = this.parse(query)
    const errors = this.validate(schema, document)

    // Queries are usually executed in batch. But after the batch is finished
    // cache just wastes memory without much benefits.
    // TODO: consider a better strategy for cache purging/invalidation
    this.scheduleClearCache()

    if (errors.length > 0) {
      return { errors }
    }

    let tracer
    if (this.graphqlTracing && parentSpan) {
      tracer = new GraphQLSpanTracer(`GraphQL Query`, {
        parentSpan,
        tags: {
          queryName: queryName,
        },
      })

      tracer.start()
    }

    try {
      // `execute` will return a promise
      return execute({
        schema,
        document,
        rootValue: context,
        contextValue: withResolverContext({
          schema,
          schemaComposer: schemaCustomization.composer,
          context,
          customContext: schemaCustomization.context,
          nodeModel: this.nodeModel,
          stats: this.stats,
          tracer,
        }),
        variableValues: context,
      })
    } finally {
      if (tracer) {
        tracer.end()
      }
Repository: gatsbyjs/gatsby
File: e2e-tests/development-runtime/cypress/plugins/block-resources.js (JavaScript)

Code before:
// this is partial copy of e2e-tests/production-runtime/cypress/plugins/block-resources.js
// partial because there is no asset blocking possibly in develop (we have single, virtual development bundle)

const fs = require(`fs-extra`)
const path = require(`path`)
const glob = require(`glob`)

const publicDir = path.join(__dirname, `..`, `..`, `public`)

const moveAsset = (from, to) => {
  const fromExists = fs.existsSync(from)
  const toExists = fs.existsSync(to)

  if (fromExists && !toExists) {
    fs.moveSync(from, to, {
      overwrite: true,
    })
  }
}

const restorePageData = hiddenPath => {
  if (path.basename(hiddenPath).charAt(0) !== `_`) {
    throw new Error(`hiddenPath should have _ prefix`)
  }
  const restoredPath = path.join(
    path.dirname(hiddenPath),
    path.basename(hiddenPath).slice(1)
  )
  moveAsset(hiddenPath, restoredPath)
}

const getPageDataPath = pagePath => {
  const fixedPagePath = pagePath === `/` ? `index` : pagePath
  return path.join(publicDir, `page-data`, fixedPagePath, `page-data.json`)
}

const getHiddenPageDataPath = pagePath => {
  const fixedPagePath = pagePath === `/` ? `index` : pagePath
  return path.join(publicDir, `page-data`, fixedPagePath, `_page-data.json`)
}

const blockPageData = pagePath =>
  moveAsset(getPageDataPath(pagePath), getHiddenPageDataPath(pagePath))

const blockAssetsForPage = ({ pagePath, filter }) => {
  if (filter === `all` || filter === `page-data`) {
    blockPageData(pagePath)
  }

  console.log(`Blocked assets for path "${pagePath}" [${filter}]`)
  return null
}

const restore = () => {
  const globPattern = path.join(publicDir, `/page-data/**`, `_page-data.json`)
  const hiddenPageDatas = glob.sync(globPattern)
  hiddenPageDatas.forEach(restorePageData)

  console.log(`Restored resources`)
  return null
}

module.exports = {
  restoreAllBlockedResources: restore,
  blockAssetsForPage,
}


Review comment (suggestion):
This can potentially cause problems if someone adds a component starting with an underscore in development-runtime? Highly unlikely though.

Code after:
  }

  console.log(`Blocked assets for path "${pagePath}" [${filter}]`)
  return null
}

function blockPageComponent({ path: pageComponentPath }) {
  const hiddenPath = path.join(
    path.dirname(pageComponentPath),
    `_` + path.basename(pageComponentPath)
  )

  moveAsset(path.join(srcDir, pageComponentPath), path.join(srcDir, hiddenPath))
  return null
}

const restore = () => {
  const hiddenPageDataGlobPattern = path.join(
    publicDir,
    `/page-data/**`,
    `_page-data.json`
  )
  const hiddenPageDatas = glob.sync(hiddenPageDataGlobPattern)
  hiddenPageDatas.forEach(restoreAsset)

  const hiddenPageComponentGlobPattern = path.join(srcDir, `**`, `_*`)
  const hiddenPageComponents = glob.sync(hiddenPageComponentGlobPattern)
  hiddenPageComponents.forEach(restoreAsset)

  console.log(`Restored resources`)
  return null
}

module.exports = {
  restoreAllBlockedResources: restore,
  blockAssetsForPage,
  blockPageComponent,
}
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/internal-plugins/prod-404/gatsby-node.js (JavaScript)

Code before:
let created404 = false
exports.onCreatePage = ({ page, store, actions }) => {
  // Copy /404/ to /404.html as many static site hosts expect
  // site 404 pages to be named this.
  // https://www.gatsbyjs.org/docs/add-404-page/
  if (!created404 && /^\/?404\/?$/.test(page.path)) {
    actions.createPage({
      ...page,
      path: `/404.html`,
    })
    created404 = true
  }
}


Review comment (suggestion):
Can this cause problems because we pass a copy of the page and not the exact same instance. I just don't know if we use strict `===` comparison for pages somewhere or not.

Code after:
const { emitter } = require(`../../redux`)
const { boundActionCreators } = require(`../../redux/actions`)

const PROD_404_PAGE_PATH = `/404.html`

let page404 = null
exports.onCreatePage = ({ page, store, actions }) => {
  // Copy /404/ to /404.html as many static site hosts expect
  // site 404 pages to be named this.
  // https://www.gatsbyjs.org/docs/add-404-page/
  if (!page404 && /^\/?404\/?$/.test(page.path)) {
    actions.createPage({
      ...page,
      path: PROD_404_PAGE_PATH,
    })
    page404 = page
  }
}

emitter.on(`DELETE_PAGE`, action => {
  if (page404 && action.payload.path === page404.path) {
    boundActionCreators.deletePage({
      ...page404,
      path: PROD_404_PAGE_PATH,
    })
    page404 = null
  }
})