ronantakizawa/codereview-bench huggingface dataset:
Repository: vitejs/vite
File: packages/vite/src/node/plugins/html.ts (TypeScript)

Code before:
  }
  return s
}

/**
 * Format parse5 @type {ParserError} to @type {RollupError}
 */
function formatParseError(parserError: ParserError, id: string, html: string) {
  const formattedError = {
    code: parserError.code,
    message: `parse5 error code ${parserError.code}`,
    frame: generateCodeFrame(
      html,
      parserError.startOffset,
      parserError.endOffset,
    ),
    loc: {
      file: id,
      line: parserError.startLine,
      column: parserError.startCol,
    },
  } satisfies RollupError
  return formattedError
}

const fileErrorInfo: Record<string, Record<string, boolean>> = {}
function handleParseError(
  parserError: ParserError,
  html: string,
  filePath: string,
  warn: Logger['warn'],
) {
  switch (parserError.code) {
    case 'missing-doctype':
      // ignore missing DOCTYPE
      return
    case 'abandoned-head-element-child':
      // Accept elements without closing tag in <head>
      return
    case 'duplicate-attribute':
      // Accept duplicate attributes #5966
      // The first attribute is used, browsers silently ignore duplicates
      return
    case 'non-void-html-element-start-tag-with-trailing-solidus':
      // Allow self closing on non-void elements #10439
      return
    case 'unexpected-question-mark-instead-of-tag-name':
      // Allow <?xml> declaration and <?> empty elements
      // lit generates <?>: https://github.com/lit/lit/issues/2470
      return
  }

Review comment (suggestion):
I think we should only dedupe if the content is same. So probably something like:
```ts
const shownWarnings: Record<string, { contentHash: string, codes: Set<string> }> = {}
const addShownWarning = (content: string, filename: string, code: string) => {
  const contentHash = hash(code)
  shownWarnings[filename] ??= { contentHash, codes: new Set() }
  shownWarnings[filename].add(code)
}
```

Code after:
  return s
}

export function removeViteIgnoreAttr(
  s: MagicString,
  sourceCodeLocation: Token.Location,
): MagicString {
  const loc = (sourceCodeLocation as Token.LocationWithAttributes).attrs?.[
    'vite-ignore'
  ]
  if (loc) {
    s.remove(loc.startOffset, loc.endOffset)
  }
  return s
}

/**
 * Format parse5 @type {ParserError} to @type {RollupError}
 */
function formatParseError(parserError: ParserError, id: string, html: string) {
  const formattedError = {
    code: parserError.code,
    message: `parse5 error code ${parserError.code}`,
    frame: generateCodeFrame(
      html,
      parserError.startOffset,
      parserError.endOffset,
    ),
    loc: {
      file: id,
      line: parserError.startLine,
      column: parserError.startCol,
    },
  } satisfies RollupError
  return formattedError
}

function handleParseError(
  parserError: ParserError,
  html: string,
  filePath: string,
  warnings: ParseWarnings,
) {
  switch (parserError.code) {
    case 'missing-doctype':
      // ignore missing DOCTYPE
      return
    case 'abandoned-head-element-child':
      // Accept elements without closing tag in <head>
      return
    case 'duplicate-attribute':
Repository: vitejs/vite
File: playground/vitestSetup.ts (TypeScript)

Code before:
    }
  }
  watcher.on('event', callback)
  await new Promise<void>((resolve) => {
    resolveFn = resolve
  })
  return watcher.off('event', callback)
}

export function createInMemoryLogger(logs: string[]): Logger {
  const loggedErrors = new WeakSet<Error | RollupError>()
  const warnedMessages = new Set<string>()

  const logger: Logger = {
    hasWarned: false,
    hasErrorLogged: (err) => loggedErrors.has(err),
    clearScreen: () => {},
    info(msg) {
      logs.push(msg)
    },
    warn(msg) {
      // for invalid HTML, we throw an error
      // to avoid false positives in tests
      if (msg.includes('Unable to parse HTML;')) {
        throw new Error(msg)
      }
      logs.push(msg)
      logger.hasWarned = true
    },
    warnOnce(msg) {
      if (warnedMessages.has(msg)) return
      logs.push(msg)
      logger.hasWarned = true
      warnedMessages.add(msg)
    },
    error(msg, opts) {
      logs.push(msg)
      if (opts?.error) {
        loggedErrors.add(opts.error)
      }
    },
  }

  return logger
}

function setupConsoleWarnCollector(logs: string[]) {
  const warn = console.warn
  console.warn = (...args) => {
    logs.push(args.join(' '))
    return warn.call(console, ...args)

Review comment (suggestion):
Could we use a custom plugin that throws an error instead of this change? It would be difficult to notice this code when writing the tests in `playground/html`.

Code after:
      previewServer.config.base,
    )
    await page.goto(viteTestUrl)
  }
}

/**
 * Send the rebuild complete message in build watch
 */
export async function notifyRebuildComplete(
  watcher: RollupWatcher,
): Promise<RollupWatcher> {
  let resolveFn: undefined | (() => void)
  const callback = (event: RollupWatcherEvent): void => {
    if (event.code === 'END') {
      resolveFn?.()
    }
  }
  watcher.on('event', callback)
  await new Promise<void>((resolve) => {
    resolveFn = resolve
  })
  return watcher.off('event', callback)
}

export function createInMemoryLogger(logs: string[]): Logger {
  const loggedErrors = new WeakSet<Error | RollupError>()
  const warnedMessages = new Set<string>()

  const logger: Logger = {
    hasWarned: false,
    hasErrorLogged: (err) => loggedErrors.has(err),
    clearScreen: () => {},
    info(msg) {
      logs.push(msg)
    },
    warn(msg) {
      logs.push(msg)
      logger.hasWarned = true
    },
    warnOnce(msg) {
      if (warnedMessages.has(msg)) return
      logs.push(msg)
      logger.hasWarned = true
      warnedMessages.add(msg)
    },
    error(msg, opts) {
      logs.push(msg)
      if (opts?.error) {
        loggedErrors.add(opts.error)
      }
Repository: vitejs/vite
File: packages/vite/src/node/__tests__/scan.spec.ts (TypeScript)

Code before:
test('scan jsx-runtime', async (ctx) => {
  const server = await createServer({
    configFile: false,
    logLevel: 'error',
    root: path.join(import.meta.dirname, 'fixtures', 'scan-jsx-runtime'),
    environments: {
      client: {
        // silence client optimizer
        optimizeDeps: {
          noDiscovery: true,
        },
      },
      ssr: {
        resolve: {
          noExternal: true,
        },
        optimizeDeps: {
          force: true,
          noDiscovery: false,
          entries: ['./entry-jsx.tsx', './entry-no-jsx.js'],
        },
      },
    },
  })

  // TODO: should server listen be needed for optimizer?
  await server.listen()
  ctx.onTestFinished(() => server.close())

  const runner = createServerModuleRunner(server.environments.ssr)

  // flush initial optimizer by importing any file
  await runner.import('./entry-no-jsx.js')

  // verify jsx won't trigger optimizer re-run
  const mod1 = await runner.import('./entry-jsx.js')
  const mod2 = await runner.import('./entry-jsx.js')
  expect((globalThis as any).__test_scan_jsx_runtime).toBe(1)
  expect(mod1).toBe(mod2)
})


Review comment (performance):
I think `server.listen` itself is not needed. The reason why it requires it now is probably:
- it is safer to start the optimizer after `buildStart` is called (https://github.com/vitejs/vite/pull/12832)
- it is safer to start the optimizer after the previous server is stopped (https://github.com/vitejs/vite/blob/557f797d29422027e8c451ca50dd84bf8c41b5f0/packages/vite/src/node/server/environment.ts#L184-L187)


Code after:
test('scan jsx-runtime', async (ctx) => {
  const server = await createServer({
    configFile: false,
    logLevel: 'error',
    root: path.join(import.meta.dirname, 'fixtures', 'scan-jsx-runtime'),
    environments: {
      client: {
        // silence client optimizer
        optimizeDeps: {
          noDiscovery: true,
        },
      },
      ssr: {
        resolve: {
          noExternal: true,
        },
        optimizeDeps: {
          force: true,
          noDiscovery: false,
          entries: ['./entry-jsx.tsx', './entry-no-jsx.js'],
        },
      },
    },
  })

  // start server to ensure optimizer run
  await server.listen()
  ctx.onTestFinished(() => server.close())

  const runner = createServerModuleRunner(server.environments.ssr, {
    hmr: { logger: false },
  })

  // flush initial optimizer by importing any file
  await runner.import('./entry-no-jsx.js')

  // verify jsx won't trigger optimizer re-run
  const mod1 = await runner.import('./entry-jsx.js')
  const mod2 = await runner.import('./entry-jsx.js')
  expect((globalThis as any).__test_scan_jsx_runtime).toBe(1)
  expect(mod1).toBe(mod2)
})
Repository: vitejs/vite
File: packages/vite/src/node/__tests__/config.spec.ts (TypeScript)

Code before:
      path.resolve(fixtures, './entry/vite.config.import-attributes.ts'),
      path.resolve(fixtures, './entry'),
    ))!
    expect(config).toMatchInlineSnapshot(`
        {
          "jsonValue": "vite",
        }
      `)
  })

  test('import.meta.main is correctly set', async () => {
    const { config } = (await loadConfigFromFile(
      {} as any,
      path.resolve(fixtures, './import-meta/vite.config.ts'),
      path.resolve(fixtures, './import-meta'),
    ))!
    expect(config).toMatchObject({
      isMain: false,
      dependencyIsMain: false,
    })
    expect(typeof config.url).toBe('string')
    expect(typeof config.dirname).toBe('string')
    expect(typeof config.filename).toBe('string')
    expect(config.url).toContain('file://')
    expect(config.dirname).toContain('import-meta')
    expect(config.filename).toContain('vite.config.ts')
  })

  describe('loadConfigFromFile with configLoader: native', () => {
    const fixtureRoot = path.resolve(fixtures, './native-import')

    test('imports a basic js config', async () => {
      const result = (await loadConfigFromFile(
        {} as any,
        path.resolve(fixtureRoot, 'basic.js'),
        fixtureRoot,
        undefined,
        undefined,
        'native',
      ))!
      expect(result.config).toMatchInlineSnapshot(`
        {
          "value": "works",
        }
      `)
      expect(result.dependencies.length).toBe(0)
    })
  })
})


Review comment (suggestion):
```suggestion
    expect(config.isMain).toBe(false)
    expect(config.url).toContain('file://')
    expect(config.dirname).toContain('import-meta')
    expect(config.filename).toContain('vite.config.ts')
```

Code after:
      path.resolve(fixtures, './entry/vite.config.import-attributes.ts'),
      path.resolve(fixtures, './entry'),
    ))!
    expect(config).toMatchInlineSnapshot(`
        {
          "jsonValue": "vite",
        }
      `)
  })

  test('import.meta.main is correctly set', async () => {
    const { config } = (await loadConfigFromFile(
      {} as any,
      path.resolve(fixtures, './import-meta/vite.config.ts'),
      path.resolve(fixtures, './import-meta'),
    ))!

    const c = config as any
    expect(c.isMain).toBe(false)
    expect(c.url).toContain('file://')
    expect(c.dirname).toContain('import-meta')
    expect(c.filename).toContain('vite.config.ts')
  })

  describe('loadConfigFromFile with configLoader: native', () => {
    const fixtureRoot = path.resolve(fixtures, './native-import')

    test('imports a basic js config', async () => {
      const result = (await loadConfigFromFile(
        {} as any,
        path.resolve(fixtureRoot, 'basic.js'),
        fixtureRoot,
        undefined,
        undefined,
        'native',
      ))!
      expect(result.config).toMatchInlineSnapshot(`
        {
          "value": "works",
        }
      `)
      expect(result.dependencies.length).toBe(0)
    })
  })
})
Repository: vitejs/vite
File: packages/vite/src/node/__tests__/config.spec.ts (TypeScript)

Code before:
    `)
  })

  test('loadConfigFromFile with import attributes', async () => {
    const { config } = (await loadConfigFromFile(
      {} as any,
      path.resolve(fixtures, './entry/vite.config.import-attributes.ts'),
      path.resolve(fixtures, './entry'),
    ))!
    expect(config).toMatchInlineSnapshot(`
        {
          "jsonValue": "vite",
        }
      `)
  })

  test('import.meta.main is correctly set', async () => {
    const { config } = (await loadConfigFromFile(
      {} as any,
      path.resolve(fixtures, './import-meta/vite.config.ts'),
      path.resolve(fixtures, './import-meta'),
    ))!
    expect(config.isMain).toBe(false)
    expect(config.url).toContain('file://')
    expect(config.dirname).toContain('import-meta')
    expect(config.filename).toContain('vite.config.ts')
  })

  describe('loadConfigFromFile with configLoader: native', () => {
    const fixtureRoot = path.resolve(fixtures, './native-import')

    test('imports a basic js config', async () => {
      const result = (await loadConfigFromFile(
        {} as any,
        path.resolve(fixtureRoot, 'basic.js'),
        fixtureRoot,
        undefined,
        undefined,
        'native',
      ))!
      expect(result.config).toMatchInlineSnapshot(`
        {
          "value": "works",
        }
      `)
      expect(result.dependencies.length).toBe(0)
    })
  })
})


Review comment (suggestion):
use `expect(config).toMatchInlineSnapshot` instead

Code after:
    `)
  })

  test('loadConfigFromFile with import attributes', async () => {
    const { config } = (await loadConfigFromFile(
      {} as any,
      path.resolve(fixtures, './entry/vite.config.import-attributes.ts'),
      path.resolve(fixtures, './entry'),
    ))!
    expect(config).toMatchInlineSnapshot(`
        {
          "jsonValue": "vite",
        }
      `)
  })

  test('import.meta.main is correctly set', async () => {
    const { config } = (await loadConfigFromFile(
      {} as any,
      path.resolve(fixtures, './import-meta/vite.config.ts'),
      path.resolve(fixtures, './import-meta'),
    ))!

    const c = config as any
    expect(c.isMain).toBe(false)
    expect(c.url).toContain('file://')
    expect(c.dirname).toContain('import-meta')
    expect(c.filename).toContain('vite.config.ts')
  })

  describe('loadConfigFromFile with configLoader: native', () => {
    const fixtureRoot = path.resolve(fixtures, './native-import')

    test('imports a basic js config', async () => {
      const result = (await loadConfigFromFile(
        {} as any,
        path.resolve(fixtureRoot, 'basic.js'),
        fixtureRoot,
        undefined,
        undefined,
        'native',
      ))!
      expect(result.config).toMatchInlineSnapshot(`
        {
          "value": "works",
        }
      `)
      expect(result.dependencies.length).toBe(0)
    })
  })
})
Repository: vitejs/vite
File: playground/lib/src/main.js (JavaScript)

Code before:
function createObject() {
  return { created: Date.now() }
}

export default function myLib(sel) {
  // Force esbuild spread helpers (https://github.com/evanw/esbuild/issues/951)
  console.log({ ...'foo' })

  document.querySelector(sel).textContent = 'It works'

  // Env vars should not be replaced
  console.log(process.env.NODE_ENV)

  // make sure umd helper has been moved to the right position
  console.log(`amd function(){ "use strict"; }`)

  // Test pure annotation preservation for tree-shaking
  // This should preserve the pure annotation comment
  const result = /* #__PURE__ */ createObject()

  return result
}

// For triggering unhandled global esbuild helpers in previous regex-based implementation for injection
;(function () {})()?.foo


Review comment (suggestion):
```suggestion
  // Pure annotation comment should be kept for ESM format
```

Code after:
export default /* @__PURE__ */ Object.assign(function myLib(sel) {
  // Force esbuild spread helpers (https://github.com/evanw/esbuild/issues/951)
  console.log({ ...'foo' })

  document.querySelector(sel).textContent = 'It works'

  // Env vars should not be replaced
  console.log(process.env.NODE_ENV)

  // make sure umd helper has been moved to the right position
  console.log(`amd function(){ "use strict"; }`)

  // eslint-disable-next-line no-debugger
  debugger
})

// For triggering unhandled global esbuild helpers in previous regex-based implementation for injection
;(function () {})()?.foo
Repository: vitejs/vite
File: playground/lib/vite.terser.config.js (JavaScript)

Code before:
import { defineConfig } from 'vite'
import baseConfig from './vite.config'

export default defineConfig({
  ...baseConfig,
  build: {
    ...baseConfig.build,
    minify: 'terser',
    terserOptions: {
      compress: {
        drop_debugger: true,
      },
    },
    outDir: 'dist/terser',
    lib: {
      ...baseConfig.build.lib,
      formats: ['es'],
    },
  },
  plugins: [],
  cacheDir: 'node_modules/.vite-terser',
})


Review comment (suggestion):
```suggestion
      ...baseConfig.build.lib,
      entry: baseConfig.build.lib.entry!,
```

Code after:
import path from 'node:path'
import { defineConfig } from 'vite'
import baseConfig from './vite.config'

export default defineConfig({
  ...baseConfig,
  build: {
    ...baseConfig.build,
    minify: 'terser',
    terserOptions: {
      compress: {
        drop_debugger: true,
      },
    },
    outDir: 'dist/terser',
    lib: {
      ...baseConfig.build.lib,
      entry: path.resolve(__dirname, 'src/main.js'),
      formats: ['es', 'iife'],
    },
  },
  plugins: [],
  cacheDir: 'node_modules/.vite-terser',
})
Repository: vitejs/vite
File: packages/vite/src/node/optimizer/index.ts (TypeScript)

Code before:
                  )
                }
              }
            }
          }
        }

        debug?.(
          `Dependencies bundled in ${(performance.now() - start).toFixed(2)}ms`,
        )

        return successfulResult
      })

      .catch((e) => {
        if (e.errors && e.message.includes('The build was canceled')) {
          // esbuild logs an error when cancelling, but this is expected so
          // return an empty result instead
          return cancelledResult
        }
        // Add context about dependency optimization to make errors clearer
        const depOptError = new Error(
          `Error during dependency optimization:\n${e.message}`,
        )
        depOptError.stack = e.stack
        throw depOptError
      })
      .finally(() => {
        return disposeContext()
      })
  })

  runResult.catch(() => {
    cleanUp()
  })

  return {
    async cancel() {
      optimizerContext.cancelled = true
      const { context } = await preparedRun
      await context?.cancel()
      cleanUp()
    },
    result: runResult,
  }
}

async function prepareEsbuildOptimizerRun(
  environment: Environment,
  depsInfo: Record<string, OptimizedDepInfo>,
  processingCacheDir: string,

Review comment (performance):
Use this code
https://github.com/vitejs/vite/blob/c12a4a76a299237a0a13b885c72fdda6e4a3c9b7/packages/vite/src/node/optimizer/scan.ts#L182-L196

Code after:
                  )
                }
              }
            }
          }
        }

        debug?.(
          `Dependencies bundled in ${(performance.now() - start).toFixed(2)}ms`,
        )

        return successfulResult
      })

      .catch(async (e) => {
        if (e.errors && e.message.includes('The build was canceled')) {
          // esbuild logs an error when cancelling, but this is expected so
          // return an empty result instead
          return cancelledResult
        }
        const prependMessage = colors.red(
          'Error during dependency optimization:\n\n',
        )
        if (e.errors) {
          const msgs = await formatMessages(e.errors, {
            kind: 'error',
            color: true,
          })
          e.message = prependMessage + msgs.join('\n')
        } else {
          e.message = prependMessage + e.message
        }
        throw e
      })
      .finally(() => {
        return disposeContext()
      })
  })

  runResult.catch(() => {
    cleanUp()
  })

  return {
    async cancel() {
      optimizerContext.cancelled = true
      const { context } = await preparedRun
      await context?.cancel()
      cleanUp()
    },
    result: runResult,
Repository: vitejs/vite
File: packages/vite/src/node/optimizer/index.ts (TypeScript)

Code before:
                    jsPath,
                    js.slice(0, js.lastIndexOf('//# sourceMappingURL=')),
                  )
                }
              }
            }
          }
        }

        debug?.(
          `Dependencies bundled in ${(performance.now() - start).toFixed(2)}ms`,
        )

        return successfulResult
      })

      .catch(async (e) => {
        if (e.errors && e.message.includes('The build was canceled')) {
          // esbuild logs an error when cancelling, but this is expected so
          // return an empty result instead
          return cancelledResult
        }
        const prependMessage = colors.red(`\
Error during dependency optimization:

`)
        if (e.errors) {
          const msgs = await formatMessages(e.errors, {
            kind: 'error',
            color: true,
          })
          e.message = prependMessage + msgs.join('\n')
        } else {
          e.message = prependMessage + e.message
        }
        throw e
      })
      .finally(() => {
        return disposeContext()
      })
  })

  runResult.catch(() => {
    cleanUp()
  })

  return {
    async cancel() {
      optimizerContext.cancelled = true
      const { context } = await preparedRun
      await context?.cancel()

Review comment (suggestion):
```suggestion
        const prependMessage = colors.red('Error during dependency optimization:\n\n')
```

I think this can be a bit shorter so it doesn't take up multiple lines, and as there's no interpolation.

Code after:
                    jsPath,
                    js.slice(0, js.lastIndexOf('//# sourceMappingURL=')),
                  )
                }
              }
            }
          }
        }

        debug?.(
          `Dependencies bundled in ${(performance.now() - start).toFixed(2)}ms`,
        )

        return successfulResult
      })

      .catch(async (e) => {
        if (e.errors && e.message.includes('The build was canceled')) {
          // esbuild logs an error when cancelling, but this is expected so
          // return an empty result instead
          return cancelledResult
        }
        const prependMessage = colors.red(
          'Error during dependency optimization:\n\n',
        )
        if (e.errors) {
          const msgs = await formatMessages(e.errors, {
            kind: 'error',
            color: true,
          })
          e.message = prependMessage + msgs.join('\n')
        } else {
          e.message = prependMessage + e.message
        }
        throw e
      })
      .finally(() => {
        return disposeContext()
      })
  })

  runResult.catch(() => {
    cleanUp()
  })

  return {
    async cancel() {
      optimizerContext.cancelled = true
      const { context } = await preparedRun
      await context?.cancel()
      cleanUp()