ronantakizawa/codereview-bench huggingface dataset:
Repository: facebook/react
File: packages/react-client/src/ReactFlightClient.js (JavaScript)

Code before:
}

function parseModel<T>(response: Response, json: UninitializedModel): T {
  const parsed = JSON.parse(json);
  // Pass a wrapper object as parentObject to match the original JSON.parse
  // reviver behavior, where the root value's reviver receives {"": rootValue}
  // as `this`. This ensures parentObject is never null when accessed downstream.
  return walkParsedJSON(response, parsed, {'': parsed}, '');
}

function walkParsedJSON(
  response: Response,
  value: JSONValue,
  parentObject: Object | null,
  key: string,
): any {
  if (typeof value === 'string') {
    if (value[0] === '$') {
      return parseModelString(response, parentObject, key, value);
    }
    return value;
  }
  if (typeof value !== 'object' || value === null) {
    return value;
  }
  if (Array.isArray(value)) {
    for (let i = 0; i < value.length; i++) {
      (value: any)[i] = walkParsedJSON(response, value[i], value, '' + i);
    }
    if (value[0] === REACT_ELEMENT_TYPE) {
      // React element tuple
      return parseModelTuple(response, value);
    }
    return value;
  }
  // Plain object
  for (const k in value) {
    if (k === __PROTO__) {
      delete (value: any)[k];
    } else {
      const walked = walkParsedJSON(response, (value: any)[k], value, k);
      if (walked !== undefined) {
        (value: any)[k] = walked;
      } else {
        delete (value: any)[k];
      }
    }
  }
  return value;
}


Review comment (refactor):
nit: for consistency we should use the imported helper (used for Flow reasons, I think)

```suggestion
  if (isArray(value)) {
```

Code after:
}

function parseModel<T>(response: Response, json: UninitializedModel): T {
  const rawModel = JSON.parse(json);
  // Pass a wrapper object as parentObject to match the original JSON.parse
  // reviver behavior, where the root value's reviver receives {"": rootValue}
  // as `this`. This ensures parentObject is never null when accessed downstream.
  return reviveModel(response, rawModel, {'': rawModel}, '');
}

function reviveModel(
  response: Response,
  value: JSONValue,
  parentObject: Object,
  key: string,
): any {
  if (typeof value === 'string') {
    if (value[0] === '$') {
      return parseModelString(response, parentObject, key, value);
    }
    return value;
  }
  if (typeof value !== 'object' || value === null) {
    return value;
  }
  if (isArray(value)) {
    for (let i = 0; i < value.length; i++) {
      (value: any)[i] = reviveModel(response, value[i], value, '' + i);
    }
    if (value[0] === REACT_ELEMENT_TYPE) {
      // React element tuple
      return parseModelTuple(response, value);
    }
    return value;
  }
  // Plain object
  for (const k in value) {
    if (k === __PROTO__) {
      delete (value: any)[k];
    } else {
      const walked = reviveModel(response, (value: any)[k], value, k);
      if (walked !== undefined) {
        (value: any)[k] = walked;
      } else {
        delete (value: any)[k];
      }
    }
  }
  return value;
}
Repository: fastify/fastify
File: lib/reply.js (JavaScript)

Code before:
    error: { type: 'string' },
    message: { type: 'string' }
  }
})

const CONTENT_TYPE = {
  JSON: 'application/json; charset=utf-8',
  PLAIN: 'text/plain; charset=utf-8',
  OCTET: 'application/octet-stream'
}
const {
  codes: {
    FST_ERR_REP_INVALID_PAYLOAD_TYPE
  }
} = require('./errors')

var getHeader

function Reply (res, context, request, log) {
  this.res = res
  this.context = context
  this.sent = false
  this._serializer = null
  this._errorHandlerCalled = false
  this._isError = false
  this._isRunningOnErroHook = false
  this.request = request
  this._headers = {}
  this._hasStatusCode = false
  this._startTime = undefined
  this.log = log
}

Reply.prototype.send = function (payload) {
  if (this._isRunningOnErroHook === true) {
    throw new Error('You cannot use `send` inside the `onError` hook')
  }

  if (this.sent) {
    this.res.log.warn({ err: new Error('Reply already sent') }, 'Reply already sent')
    return
  }

  this.sent = true

  if (payload instanceof Error || this._isError === true) {
    onErrorHook(this, payload, onSendHook)
    return
  }

  if (payload === undefined) {

Review comment (suggestion):
`_isRunningOnErroHook` you missing an `r` here in error which trickles down to all usages

Code after:
const wrapThenable = require('./wrapThenable')

const serializeError = FJS({
  type: 'object',
  properties: {
    statusCode: { type: 'number' },
    code: { type: 'string' },
    error: { type: 'string' },
    message: { type: 'string' }
  }
})

const CONTENT_TYPE = {
  JSON: 'application/json; charset=utf-8',
  PLAIN: 'text/plain; charset=utf-8',
  OCTET: 'application/octet-stream'
}
const {
  codes: {
    FST_ERR_REP_INVALID_PAYLOAD_TYPE
  }
} = require('./errors')

var getHeader

function Reply (res, context, request, log) {
  this.res = res
  this.context = context
  this.sent = false
  this[kReplySerializer] = null
  this[kReplyErrorHandlerCalled] = false
  this[kReplyIsError] = false
  this[kReplyIsRunningOnErrorHook] = false
  this.request = request
  this[kReplyHeaders] = {}
  this[kReplyHasStatusCode] = false
  this[kReplyStartTime] = undefined
  this.log = log
}

Reply.prototype.send = function (payload) {
  if (this[kReplyIsRunningOnErrorHook] === true) {
    throw new Error('You cannot use `send` inside the `onError` hook')
  }

  if (this.sent) {
    this.res.log.warn({ err: new Error('Reply already sent') }, 'Reply already sent')
    return
  }

  this.sent = true
Repository: fastify/fastify
File: types/type-provider.d.ts (TypeScript)

Code before:
export interface FastifyTypeProvider {
  readonly input: unknown,
  readonly output: unknown,
}

export interface FastifyTypeProviderDefault extends FastifyTypeProvider {
  output: unknown
}

export type CallTypeProvider<F extends FastifyTypeProvider, I> = (F & { input: I })['output']


Review comment (suggestion):
was this missed? will anything be done about it?

we thought to get rid of some unnecessary `async` in our code, but then any `return reply.send()` was flagged as a type error. Having to wrap in `Promise.resolve(reply.send())` (or moving the `return` to another line) isn't quite as ergonomical 😅 

Code after:
// Resolves the Reply type either via generic argument or from response schema. This type uses a different
// resolution strategy to Requests where the Reply will infer a union of each status code type specified
// by the user. The Reply can be explicitly overriden by users providing a generic Reply type on the route.
export type ResolveFastifyReplyType<TypeProvider extends FastifyTypeProvider, SchemaCompiler extends FastifySchema, RouteGeneric extends RouteGenericInterface> = FastifyReplyType<
UseReplyFromRouteGeneric<RouteGeneric> extends true ? ResolveReplyFromRouteGeneric<RouteGeneric> :
  UseReplyFromSchemaCompiler<SchemaCompiler> extends true ? ResolveReplyFromSchemaCompiler<TypeProvider, SchemaCompiler> :
    unknown
>

// -----------------------------------------------------------------------------------------------
// FastifyReplyReturnType
// -----------------------------------------------------------------------------------------------

// The target reply return type. This type is inferenced on fastify 'routes' via generic argument assignment
export type ResolveFastifyReplyReturnType<
  TypeProvider extends FastifyTypeProvider,
  SchemaCompiler extends FastifySchema,
  RouteGeneric extends RouteGenericInterface,
> = ResolveFastifyReplyType<
TypeProvider,
SchemaCompiler,
RouteGeneric
> extends infer Return ?
  (void | Promise<Return | void>)
// review: support both async and sync return types
// (Promise<Return> | Return | Promise<void> | void)
  : unknown
Repository: fastify/fastify
File: test/404s.test.js (JavaScript)

Code before:
      reply.send('valid route')
    })

    fastify.inject({
      url: '/invalid-route',
      method: 'GET'
    }, (err, response) => {
      t.error(err)
      t.equal(response.statusCode, 404)
      t.equal(response.payload, 'this was not found')
    })

    fastify.inject({
      url: '/valid-route',
      method: 'GET'
    }, (err, response) => {
      t.error(err)
      t.equal(response.statusCode, 200)
      t.equal(response.payload, 'valid route')
    })
  })

  t.end()
})

test('Should fail to invoke callNotFound inside a 404 handler', t => {
  t.plan(2)

  let fastify = null
  try {
    fastify = Fastify({
      frameworkErrors: (error, req, reply) => {
        return reply.callNotFound(error)
      }
    })
  } catch (e) {
    t.fail()
  }

  fastify.inject({
    url: '/%',
    method: 'GET'
  }, (err, res) => {
    t.error(err)
    t.equal(res.statusCode, 404)
  })
})


Review comment (suggestion):
```suggestion
test('Should handle to invoke callNotFound inside a 404 handler', t => {
```

Code after:
      reply.send('valid route')
    })

    fastify.inject({
      url: '/invalid-route',
      method: 'GET'
    }, (err, response) => {
      t.error(err)
      t.equal(response.statusCode, 404)
      t.equal(response.payload, 'this was not found')
    })

    fastify.inject({
      url: '/valid-route',
      method: 'GET'
    }, (err, response) => {
      t.error(err)
      t.equal(response.statusCode, 200)
      t.equal(response.payload, 'valid route')
    })
  })

  t.end()
})

test('Send 404 when frameworkError calls reply.callNotFound', t => {
  t.test('Dynamic route', t => {
    t.plan(4)
    const fastify = Fastify({
      logger: true,
      frameworkErrors: (error, req, reply) => {
        t.equal(error.message, "'%world' is not a valid url component")
        return reply.callNotFound()
      }
    })
    fastify.get('/hello/:id', () => t.fail('we should not be here'))
    fastify.inject({
      url: '/hello/%world',
      method: 'GET'
    }, (err, response) => {
      t.error(err)
      t.equal(response.statusCode, 404)
      t.equal(response.payload, '404 Not Found')
    })
  })

  t.end()
})
Repository: fastify/fastify
File: lib/contentTypeParser.js (JavaScript)

Code before:
  if (this[kState].started) {
    throw new Error('Cannot call "removeContentTypeParser" when fastify instance is already started!')
  }

  if (Array.isArray(contentType)) {
    for (const type of contentType) {
      this[kContentTypeParser].remove(type)
    }
  } else {
    this[kContentTypeParser].remove(contentType)
  }
}

function removeAllContentTypeParsers () {
  if (this[kState].started) {
    throw new Error('Cannot call "removeAllContentTypeParsers" when fastify instance is already started!')
  }

  this[kContentTypeParser].removeAll()
}

function validateRegExp (regexp) {
  // RegExp should either start with ^ or include ;?
  // It can ensure the user is properly detect the essence
  // MIME types.
  if (regexp.source[0] !== '^' && !regexp.source.includes(';?')) {
    warning.emit('FSTSEC001', regexp.source)
  }
}

module.exports = ContentTypeParser
module.exports.helpers = {
  buildContentTypeParser,
  addContentTypeParser,
  hasContentTypeParser,
  removeContentTypeParser,
  removeAllContentTypeParsers
}
module.exports.defaultParsers = {
  getDefaultJsonParser,
  defaultTextParser: defaultPlainTextParser
}
module.exports[kTestInternals] = { rawBody }


Review comment (suggestion):
```suggestion
  if (regexp.source[0] !== '^' && regexp.source.includes(';?') === true) {
```

Code after:

  if (Array.isArray(contentType)) {
    contentType.forEach((type) => this[kContentTypeParser].add(type, opts, parser))
  } else {
    this[kContentTypeParser].add(contentType, opts, parser)
  }

  return this
}

function hasContentTypeParser (contentType) {
  return this[kContentTypeParser].hasParser(contentType)
}

function removeContentTypeParser (contentType) {
  if (this[kState].started) {
    throw new FST_ERR_CTP_INSTANCE_ALREADY_STARTED('removeContentTypeParser')
  }

  if (Array.isArray(contentType)) {
    for (const type of contentType) {
      this[kContentTypeParser].remove(type)
    }
  } else {
    this[kContentTypeParser].remove(contentType)
  }
}

function removeAllContentTypeParsers () {
  if (this[kState].started) {
    throw new FST_ERR_CTP_INSTANCE_ALREADY_STARTED('removeAllContentTypeParsers')
  }

  this[kContentTypeParser].removeAll()
}

function validateRegExp (regexp) {
  // RegExp should either start with ^ or include ;?
  // It can ensure the user is properly detect the essence
  // MIME types.
  if (regexp.source[0] !== '^' && regexp.source.includes(';?') === false) {
    FSTSEC001(regexp.source)
  }
}

module.exports = ContentTypeParser
module.exports.helpers = {
  buildContentTypeParser,
  addContentTypeParser,
  hasContentTypeParser,
  removeContentTypeParser,
Repository: fastify/fastify
File: lib/warnings.js (JavaScript)

Code before:
'use strict'

const warning = require('process-warning')()

warning.create('FastifySecurity', 'FSTSEC001', 'You are using /%s/ Content-Type which may vulnerable to CORS attack. Please make sure your RegExp start with "^" or include ";?" to proper detect the essence MIME type.')

/**
 * Deprecation codes:
 *   - FSTDEP005
 */

warning.create('FastifyDeprecation', 'FSTDEP005', 'You are accessing the deprecated "request.connection" property. Use "request.socket" instead.')

warning.create('FastifyDeprecation', 'FSTDEP006', 'You are decorating Request/Reply with a reference type. This reference is shared amongst all requests. Use onRequest hook instead. Property: %s')

warning.create('FastifyDeprecation', 'FSTDEP007', 'You are trying to set a HEAD route using "exposeHeadRoute" route flag when a sibling route is already set. See documentation for more info.')

warning.create('FastifyDeprecation', 'FSTDEP008', 'You are using route constraints via the route { version: "..." } option, use { constraints: { version: "..." } } option instead.')

warning.create('FastifyDeprecation', 'FSTDEP009', 'You are using a custom route versioning strategy via the server { versioning: "..." } option, use { constraints: { version: "..." } } option instead.')

warning.create('FastifyDeprecation', 'FSTDEP010', 'Modifying the "reply.sent" property is deprecated. Use the "reply.hijack()" method instead.')

warning.create('FastifyDeprecation', 'FSTDEP011', 'Variadic listen method is deprecated. Please use ".listen(optionsObject)" instead. The variadic signature will be removed in `fastify@5`.')

warning.create('FastifyDeprecation', 'FSTDEP012', 'Request#context property access is deprecated. Please use "Request#routeConfig" or "Request#routeSchema" instead for accessing Route settings. The "Request#context" will be removed in `fastify@5`.')

warning.create('FastifyDeprecation', 'FSTDEP013', 'Direct return of "trailers" function is deprecated. Please use "callback" or "async-await" for return value. The support of direct return will removed in `fastify@5`.')

module.exports = warning

Review comment (security):
```suggestion
warning.create('FastifySecurity', 'FSTSEC001', 'You are using /%s/ Content-Type which may be vulnerable to CORS attack. Please make sure your RegExp start with "^" or include ";?" to proper detection of the essence MIME type.')
```

Code after:
'use strict'

const { createDeprecation, createWarning } = require('process-warning')

/**
 * Deprecation codes:
 *   - FSTDEP005
 *   - FSTDEP006
 *   - FSTDEP007
 *   - FSTDEP008
 *   - FSTDEP009
 *   - FSTDEP010
 *   - FSTDEP012
 *   - FSTDEP013
 *   - FSTDEP015
 *   - FSTDEP016
 *   - FSTDEP017
 *   - FSTDEP018
 *   - FSTDEP019
 *   - FSTWRN001
 *   - FSTSEC001
 */

const FSTDEP005 = createDeprecation({
  code: 'FSTDEP005',
  message: 'You are accessing the deprecated "request.connection" property. Use "request.socket" instead.'
})

const FSTDEP006 = createDeprecation({
  code: 'FSTDEP006',
Repository: fastify/fastify
File: test/default-route.test.js (JavaScript)

Code before:
  fastify.getDefaultRoute()
})

test('should fail if defaultRoute is not a function', t => {
  t.plan(1)

  const fastify = Fastify()
  const defaultRoute = {}

  fastify.get('/', () => {})

  try {
    fastify.setDefaultRoute(defaultRoute)
  } catch (error) {
    t.equal(error.code, 'FST_ERR_DEFAULT_ROUTE_INVALID_TYPE')
  }
})

test('correctly sets, returns, and calls defaultRoute', t => {
  t.plan(3)

  const fastify = Fastify()
  const defaultRoute = (req, res) => {
    res.end('hello from defaultRoute')
  }

  fastify.setDefaultRoute(defaultRoute)
  const returnedDefaultRoute = fastify.getDefaultRoute()
  t.equal(returnedDefaultRoute, defaultRoute)

  fastify.get('/', () => {})

  fastify.inject({
    method: 'GET',
    url: '/random'
  }, (err, res) => {
    t.error(err)
    t.equal(res.body, 'hello from defaultRoute')
  })
})


Review comment (performance):
Due to the nature of `process-warning`. It won't emit a second warning for the same code, so I can't test `getDefaultRoute` after `setDefaultRoute` (or vice-versa). A possible solution would be spawning a new process or deleting `require.cache[path.resolve('../fastify.js']` and importing it again. 

However, I don't think that's necessary,  so I've just included the second call to avoid a future mistake. 

Code after:

  t.teardown(() => {
    process.removeListener('warning', onWarning)
    warning.emitted.set('FSTDEP014', false)
  })

  fastify.setDefaultRoute(defaultRoute)
})

test('getDefaultRoute should emit a deprecation warning', t => {
  t.plan(2)

  const fastify = Fastify()

  process.on('warning', onWarning)
  function onWarning (warning) {
    t.equal(warning.name, 'FastifyDeprecation')
    t.equal(warning.code, 'FSTDEP014')
  }

  t.teardown(() => {
    process.removeListener('warning', onWarning)
    warning.emitted.set('FSTDEP014', false)
  })

  fastify.getDefaultRoute()
})

test('should fail if defaultRoute is not a function', t => {
  t.plan(1)

  const fastify = Fastify()
  const defaultRoute = {}

  fastify.get('/', () => {})

  try {
    fastify.setDefaultRoute(defaultRoute)
  } catch (error) {
    t.equal(error.code, 'FST_ERR_DEFAULT_ROUTE_INVALID_TYPE')
  }
})

test('correctly sets, returns, and calls defaultRoute', t => {
  t.plan(3)

  const fastify = Fastify()
  const defaultRoute = (req, res) => {
    res.end('hello from defaultRoute')
  }
Repository: fastify/fastify
File: types/utils.d.ts (TypeScript)

Code before:
export type ReplyKeysToCodes<Key> = [Key] extends [never] ? number :
  Key extends HttpCodes ? Key :
    Key extends `${infer X extends CodeClasses}xx` ?
      StringAsNumber<`${X}${Digit}${Digit}`> : number;

export type CodeToReplyKey<Code extends number> = `${Code}` extends `${infer FirstDigit extends CodeClasses}${number}`
  ? `${FirstDigit}xx`
  : never;

export type RecordKeysToLowercase<Input> = Input extends Record<string, unknown>
  ? {
    [Key in keyof Input as Key extends string
      ? Lowercase<Key>
      : Key
    ]: Input[Key];
  }
  : Input;

type OmitIndexSignature<T> = {
  [K in keyof T as string extends K ? never : number extends K ? never : K]: T[K];
};

/**
 * HTTP header strings
 */
export type HttpHeader = keyof OmitIndexSignature<http.OutgoingHttpHeaders> | (string & {}); // eslint-disable-line @typescript-eslint/ban-types


Review comment (suggestion):
Please add a comment to warn for the usage on return value.
This hack is perfectly fine for `input` value but not for `return` value.
https://github.com/microsoft/TypeScript/issues/29729#issuecomment-554927299

Code after:
export type ReplyKeysToCodes<Key> = [Key] extends [never] ? number :
  Key extends HttpCodes ? Key :
    Key extends `${infer X extends CodeClasses}xx` ?
      StringAsNumber<`${X}${Digit}${Digit}`> : number;

export type CodeToReplyKey<Code extends number> = `${Code}` extends `${infer FirstDigit extends CodeClasses}${number}`
  ? `${FirstDigit}xx`
  : never;

export type RecordKeysToLowercase<Input> = Input extends Record<string, unknown>
  ? {
    [Key in keyof Input as Key extends string
      ? Lowercase<Key>
      : Key
    ]: Input[Key];
  }
  : Input;

type OmitIndexSignature<T> = {
  [K in keyof T as string extends K ? never : number extends K ? never : K]: T[K];
};

/**
 * HTTP header strings
 * Use this type only for input values, not for output values.
 */
export type HttpHeader = keyof OmitIndexSignature<http.OutgoingHttpHeaders> | (string & Record<never, never>);
Repository: fastify/fastify
File: fastify.js (JavaScript)

Code before:
            return
          }
          httpHandler(req, res)
        })
      }, opts)
    }
  }

  function ready (cb) {
    if (this[kState].readyPromise !== null) {
      if (cb != null) {
        this[kState].readyPromise.then(() => cb(null, fastify), cb)
        return
      }

      return this[kState].readyPromise
    }

    let resolveReady
    let rejectReady

    // run the hooks after returning the promise
    process.nextTick(runHooks)

    if (!cb) {
      const promise = new Promise(function (resolve, reject) {
        resolveReady = resolve
        rejectReady = reject
      })

      this[kState].readyPromise = promise

      return promise
    }

    function runHooks () {
      // start loading
      fastify[kAvvioBoot]((err, done) => {
        if (err || fastify[kState].started || fastify[kState].ready || fastify[kState].booting) {
          manageErr(err)
        } else {
          fastify[kState].booting = true
          hookRunnerApplication('onReady', fastify[kAvvioBoot], fastify, manageErr)
        }
        done()
      })
    }

    function manageErr (err) {
      // If the error comes out of Avvio's Error codes
      // We create a make and preserve the previous error

Review comment (suggestion):
```suggestion
      this[kState].readyPromise = new Promise(function (resolve, reject) {
```

Code after:
            return
          }
          httpHandler(req, res)
        })
      }, opts)
    }
  }

  function ready (cb) {
    if (this[kState].readyPromise !== null) {
      if (cb != null) {
        this[kState].readyPromise.then(() => cb(null, fastify), cb)
        return
      }

      return this[kState].readyPromise
    }

    let resolveReady
    let rejectReady

    // run the hooks after returning the promise
    process.nextTick(runHooks)

    // Create a promise no matter what
    // It will work as a barrier for all the .ready() calls (ensuring single hook execution)
    // as well as a flow control mechanism to chain cbs and further
    // promises
    this[kState].readyPromise = new Promise(function (resolve, reject) {
      resolveReady = resolve
      rejectReady = reject
    })

    if (!cb) {
      return this[kState].readyPromise
    } else {
      this[kState].readyPromise.then(() => cb(null, fastify), cb)
    }

    function runHooks () {
      // start loading
      fastify[kAvvioBoot]((err, done) => {
        if (err || fastify[kState].started || fastify[kState].ready || fastify[kState].booting) {
          manageErr(err)
        } else {
          fastify[kState].booting = true
          hookRunnerApplication('onReady', fastify[kAvvioBoot], fastify, manageErr)
        }
        done()
      })
    }