ronantakizawa/codereview-bench huggingface dataset:
Repository: reduxjs/redux
File: rollup.config.js (JavaScript)

Code before:
import nodeResolve from 'rollup-plugin-node-resolve'
import babel from 'rollup-plugin-babel'
import replace from 'rollup-plugin-replace'
import { terser } from 'rollup-plugin-terser'

import pkg from './package.json'

const peersExternal = Object.keys(pkg.peerDependencies || {})
const allExternal = peersExternal.concat(Object.keys(pkg.dependencies || {}))

const makeExternalPredicate = externalArr => {
  if (externalArr.length === 0) {
    return () => false
  }
  const pattern = new RegExp(`^(${externalArr.join('|')})($|/)`)
  return id => pattern.test(id)
}

const changeExtension = (filename, extension) => filename.replace(/\..+$/i, extension)

const createConfig = ({
  output,
  external = 'all',
  transpile = true,
  min = false,
  environment,
}) => ({
  input: 'src/index.js',
  output: Object.assign({
    name: 'Redux',
    indent: false,
  }, output),
  external: makeExternalPredicate(external === 'peers' ? peersExternal : allExternal),
  plugins: [
    nodeResolve({
      jsnext: true
    }),
    transpile && babel({
      exclude: 'node_modules/**',
      plugins: [
        ['@babel/transform-runtime', {
          useESModules: output.format !== 'cjs'
        }]
      ],
      runtimeHelpers: true,
    }),

Review comment (suggestion):
I don't want to use factory functions here. Please keep it as a simple config file. It's harder to read through and figure out what the actual output is this way. Yes, it's more verbose, but it's also much more approachable this way.

Code after:
import nodeResolve from 'rollup-plugin-node-resolve'
import babel from 'rollup-plugin-babel'
import replace from 'rollup-plugin-replace'
import typescript from 'rollup-plugin-typescript2'
import { terser } from 'rollup-plugin-terser'

import pkg from './package.json'

const noDeclarationFiles = { compilerOptions: { declaration: false } }

const babelRuntimeVersion = pkg.dependencies['@babel/runtime'].replace(
  /^[^0-9]*/,
  ''
)

const makeExternalPredicate = externalArr => {
  if (externalArr.length === 0) {
    return () => false
  }
  const pattern = new RegExp(`^(${externalArr.join('|')})($|/)`)
  return id => pattern.test(id)
}

export default [
  // CommonJS
  {
    input: 'src/index.ts',
    output: { file: 'lib/redux.js', format: 'cjs', indent: false },
    external: makeExternalPredicate([
      ...Object.keys(pkg.dependencies || {}),
      ...Object.keys(pkg.peerDependencies || {})
    ]),
    plugins: [
      nodeResolve({
        extensions: ['.ts']
      }),
      typescript({ useTsconfigDeclarationDir: true }),
      babel({
        plugins: [
          ['@babel/plugin-transform-runtime', { version: babelRuntimeVersion }]
        ],
        runtimeHelpers: true
      })
    ]
  },
Repository: reduxjs/redux
File: .eslintrc.js (JavaScript)

Code before:
    },
    'import/resolver': {
      // use <root>/tsconfig.json
      typescript: {}
    }
  },

  rules: {
    'jsx-a11y/href-no-hash': 'off',
    'no-unused-vars': 'off',
    '@typescript-eslint/no-unused-vars': [
      'error',
      {
        vars: 'all',
        args: 'after-used',
        ignoreRestSiblings: true,
        argsIgnorePattern: '^_' // ignore unused variables whose name is '_'
      }
    ]
  },

  overrides: [
    {
      files: 'test/typescript/*.ts',
      rules: {
        'no-unused-expressions': 'off',
        '@typescript-eslint/no-namespace': 'off',
        '@typescript-eslint/no-unused-vars': 'off'
      }
    }
  ]
}


Review comment (suggestion):
I removed this because it's no longer necessary, as all the test files are now in TypeScript, and TypeScript will make sure the Jest globals are used properly

Code after:
module.exports = {
  extends: 'react-app',

  parser: '@typescript-eslint/parser',

  plugins: ['@typescript-eslint'],

  settings: {
    react: {
      version: '16.8'
    },
    'import/parsers': {
      '@typescript-eslint/parser': ['.ts', '.tsx']
    },
    'import/resolver': {
      // use <root>/tsconfig.json
      typescript: {}
    }
  },

  rules: {
    'jsx-a11y/href-no-hash': 'off',
    'no-unused-vars': 'off',
    '@typescript-eslint/no-unused-vars': [
      'error',
      {
        vars: 'all',
        args: 'after-used',
        ignoreRestSiblings: true,
        argsIgnorePattern: '^_' // ignore unused variables whose name is '_'
      }
    ]
  }
}
Repository: reduxjs/redux
File: .eslintrc.js (JavaScript)

Code before:
      // use <root>/tsconfig.json
      typescript: {}
    }
  },

  rules: {
    'jsx-a11y/href-no-hash': 'off',
    'no-unused-vars': 'off',
    '@typescript-eslint/no-unused-vars': [
      'error',
      {
        vars: 'all',
        args: 'after-used',
        ignoreRestSiblings: true,
        argsIgnorePattern: '^_' // ignore unused variables whose name is '_'
      }
    ]
  },

  overrides: [
    {
      files: 'test/typescript/*.ts',
      rules: {
        'no-unused-expressions': 'off',
        '@typescript-eslint/no-namespace': 'off',
        '@typescript-eslint/no-unused-vars': 'off'
      }
    }
  ]
}


Review comment (suggestion):
These rules aren't useful for the TypeScript tests because the code doesn't actually run, it's just meant to be typechecked

Code after:
module.exports = {
  extends: 'react-app',

  parser: '@typescript-eslint/parser',

  plugins: ['@typescript-eslint'],

  settings: {
    react: {
      version: '16.8'
    },
    'import/parsers': {
      '@typescript-eslint/parser': ['.ts', '.tsx']
    },
    'import/resolver': {
      // use <root>/tsconfig.json
      typescript: {}
    }
  },

  rules: {
    'jsx-a11y/href-no-hash': 'off',
    'no-unused-vars': 'off',
    '@typescript-eslint/no-unused-vars': [
      'error',
      {
        vars: 'all',
        args: 'after-used',
        ignoreRestSiblings: true,
        argsIgnorePattern: '^_' // ignore unused variables whose name is '_'
      }
    ]
  }
}
Repository: reduxjs/redux
File: test/applyMiddleware.spec.ts (TypeScript)

Code before:
    store.dispatch(addTodo('Use Redux'))
    store.dispatch(addTodo('Flux FTW!'))

    expect(spy.mock.calls.length).toEqual(1)

    expect(spy.mock.calls[0][0]).toHaveProperty('getState')
    expect(spy.mock.calls[0][0]).toHaveProperty('dispatch')

    expect(store.getState()).toEqual([
      { id: 1, text: 'Use Redux' },
      { id: 2, text: 'Flux FTW!' }
    ])
  })

  it('passes recursive dispatches through the middleware chain', async () => {
    function test(spyOnMethods: jest.Mock) {
      return () => next => action => {
        spyOnMethods(action)
        return next(action)
      }
    }

    const spy = jest.fn()
    const store = applyMiddleware(test(spy), thunk)(createStore)(reducers.todos)

    await store.dispatch(addTodoAsync('Use Redux'))
    expect(spy.mock.calls.length).toEqual(2);
  })

  it('works with thunk middleware', async () => {
    const store = applyMiddleware(thunk)(createStore)(reducers.todos)

    store.dispatch(addTodoIfEmpty('Hello'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
      }
    ])

    store.dispatch(addTodoIfEmpty('Hello'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
      }
    ])

    store.dispatch(addTodo('World'))
    expect(store.getState()).toEqual([
      {

Review comment (suggestion):
This is why I installed `redux-thunk` as a dev dependency. I think it makes sense to ensure its compatibility here, seeing as the tests are using thunks.

Code after:
    store.dispatch(addTodo('Use Redux'))
    store.dispatch(addTodo('Flux FTW!'))

    expect(spy.mock.calls.length).toEqual(1)

    expect(spy.mock.calls[0][0]).toHaveProperty('getState')
    expect(spy.mock.calls[0][0]).toHaveProperty('dispatch')

    expect(store.getState()).toEqual([
      { id: 1, text: 'Use Redux' },
      { id: 2, text: 'Flux FTW!' }
    ])
  })

  it('passes recursive dispatches through the middleware chain', () => {
    function test(spyOnMethods) {
      return () => next => action => {
        spyOnMethods(action)
        return next(action)
      }
    }

    const spy = jest.fn()
    const store = applyMiddleware(test(spy), thunk)(createStore)(reducers.todos)

    // the typing for redux-thunk is super complex, so we will use an as unknown hack
    const dispatchedValue = (store.dispatch(
      addTodoAsync('Use Redux')
    ) as unknown) as Promise<void>
    return dispatchedValue.then(() => {
      expect(spy.mock.calls.length).toEqual(2)
    })
  })

  it('works with thunk middleware', done => {
    const store = applyMiddleware(thunk)(createStore)(reducers.todos)

    store.dispatch(addTodoIfEmpty('Hello'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
      }
    ])

    store.dispatch(addTodoIfEmpty('Hello'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
      }
Repository: reduxjs/redux
File: test/applyMiddleware.spec.ts (TypeScript)

Code before:
    const store = applyMiddleware(test(spy), thunk)(createStore)(reducers.todos)

    store.dispatch(addTodo('Use Redux'))
    store.dispatch(addTodo('Flux FTW!'))

    expect(spy.mock.calls.length).toEqual(1)

    expect(spy.mock.calls[0][0]).toHaveProperty('getState')
    expect(spy.mock.calls[0][0]).toHaveProperty('dispatch')

    expect(store.getState()).toEqual([
      { id: 1, text: 'Use Redux' },
      { id: 2, text: 'Flux FTW!' }
    ])
  })

  it('passes recursive dispatches through the middleware chain', async () => {
    function test(spyOnMethods: jest.Mock) {
      return () => next => action => {
        spyOnMethods(action)
        return next(action)
      }
    }

    const spy = jest.fn()
    const store = applyMiddleware(test(spy), thunk)(createStore)(reducers.todos)

    await store.dispatch(addTodoAsync('Use Redux'))
    expect(spy.mock.calls.length).toEqual(2);
  })

  it('works with thunk middleware', async () => {
    const store = applyMiddleware(thunk)(createStore)(reducers.todos)

    store.dispatch(addTodoIfEmpty('Hello'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
      }
    ])

    store.dispatch(addTodoIfEmpty('Hello'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
      }
    ])

    store.dispatch(addTodo('World'))

Review comment (suggestion):
This `store` was not being typed properly, because the `thunk` wasn't typed properly. That's now fixed in this PR.

Code after:
    const store = applyMiddleware(test(spy), thunk)(createStore)(reducers.todos)

    store.dispatch(addTodo('Use Redux'))
    store.dispatch(addTodo('Flux FTW!'))

    expect(spy.mock.calls.length).toEqual(1)

    expect(spy.mock.calls[0][0]).toHaveProperty('getState')
    expect(spy.mock.calls[0][0]).toHaveProperty('dispatch')

    expect(store.getState()).toEqual([
      { id: 1, text: 'Use Redux' },
      { id: 2, text: 'Flux FTW!' }
    ])
  })

  it('passes recursive dispatches through the middleware chain', () => {
    function test(spyOnMethods) {
      return () => next => action => {
        spyOnMethods(action)
        return next(action)
      }
    }

    const spy = jest.fn()
    const store = applyMiddleware(test(spy), thunk)(createStore)(reducers.todos)

    // the typing for redux-thunk is super complex, so we will use an as unknown hack
    const dispatchedValue = (store.dispatch(
      addTodoAsync('Use Redux')
    ) as unknown) as Promise<void>
    return dispatchedValue.then(() => {
      expect(spy.mock.calls.length).toEqual(2)
    })
  })

  it('works with thunk middleware', done => {
    const store = applyMiddleware(thunk)(createStore)(reducers.todos)

    store.dispatch(addTodoIfEmpty('Hello'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
      }
    ])

    store.dispatch(addTodoIfEmpty('Hello'))
    expect(store.getState()).toEqual([
      {
        id: 1,
Repository: reduxjs/redux
File: test/applyMiddleware.spec.ts (TypeScript)

Code before:

    expect(store.getState()).toEqual([
      { id: 1, text: 'Use Redux' },
      { id: 2, text: 'Flux FTW!' }
    ])
  })

  it('passes recursive dispatches through the middleware chain', async () => {
    function test(spyOnMethods: jest.Mock) {
      return () => next => action => {
        spyOnMethods(action)
        return next(action)
      }
    }

    const spy = jest.fn()
    const store = applyMiddleware(test(spy), thunk)(createStore)(reducers.todos)

    await store.dispatch(addTodoAsync('Use Redux'))
    expect(spy.mock.calls.length).toEqual(2);
  })

  it('works with thunk middleware', async () => {
    const store = applyMiddleware(thunk)(createStore)(reducers.todos)

    store.dispatch(addTodoIfEmpty('Hello'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
      }
    ])

    store.dispatch(addTodoIfEmpty('Hello'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
      }
    ])

    store.dispatch(addTodo('World'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
      },
      {
        id: 2,
        text: 'World'
      }

Review comment (suggestion):
Now that the `store` is typed properly, this `dispatch` call has no issues. The root of the problem has been fixed.

Code after:

    expect(store.getState()).toEqual([
      { id: 1, text: 'Use Redux' },
      { id: 2, text: 'Flux FTW!' }
    ])
  })

  it('passes recursive dispatches through the middleware chain', () => {
    function test(spyOnMethods) {
      return () => next => action => {
        spyOnMethods(action)
        return next(action)
      }
    }

    const spy = jest.fn()
    const store = applyMiddleware(test(spy), thunk)(createStore)(reducers.todos)

    // the typing for redux-thunk is super complex, so we will use an as unknown hack
    const dispatchedValue = (store.dispatch(
      addTodoAsync('Use Redux')
    ) as unknown) as Promise<void>
    return dispatchedValue.then(() => {
      expect(spy.mock.calls.length).toEqual(2)
    })
  })

  it('works with thunk middleware', done => {
    const store = applyMiddleware(thunk)(createStore)(reducers.todos)

    store.dispatch(addTodoIfEmpty('Hello'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
      }
    ])

    store.dispatch(addTodoIfEmpty('Hello'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
      }
    ])

    store.dispatch(addTodo('World'))
    expect(store.getState()).toEqual([
      {
        id: 1,
        text: 'Hello'
Repository: reduxjs/redux
File: test/helpers/middleware.ts (TypeScript)

Code before:
import { ThunkDispatch } from 'redux-thunk'

import { rootActionTypes } from './actionTypes'
import { Middleware } from '../..';

export const thunk: Middleware<
  ThunkDispatch<any, {}, rootActionTypes>,
  any,
  ThunkDispatch<any, {}, rootActionTypes>
> = (
  { dispatch, getState },
) => {
  return (next) => (action) =>
    typeof action === 'function' ? action(dispatch, getState) : next(action)
}


Review comment (suggestion):
Unfortunately, using `ThunkMiddleware` kept returning a type of `any` for some reason, so I resorted to this redux `Middleware` type instead, which gives us a properly-typed `thunk`. Worth looking into in the `redux-thunk` library why this is happening, but outside the scope of this PR.

Code after:
import { MiddlewareAPI, Dispatch, AnyAction } from '../..'

type ThunkAction<T extends any = any> = T extends AnyAction
  ? AnyAction
  : T extends Function
  ? T
  : never

export function thunk({ dispatch, getState }: MiddlewareAPI) {
  return (next: Dispatch) => <T>(action: ThunkAction) =>
    typeof action === 'function' ? action(dispatch, getState) : next(action)
}
Repository: reduxjs/redux
File: test/typescript/enhancers.ts (TypeScript)

Code before:
    extraField: 'extra'
  }

  const enhancer: StoreEnhancer<{}, ExtraState> = createStore => <
    S,
    A extends Action = AnyAction
  >(
    reducer: Reducer<S, A>,
    preloadedState?: any
  ) => {
    const wrapReducer = (
      reducer: Reducer<S, A>
    ): Reducer<S & ExtraState, A> => (state, action) => {
      const newState = reducer(state, action)
      return {
        ...newState,
        extraField: 'extra'
      }
    }
    const wrappedPreloadedState = preloadedState
      ? {
          ...preloadedState,
          extraField: 'extra'
        }
      : undefined
    const store = createStore(wrapReducer(reducer), wrappedPreloadedState)
    return {
      ...store,
      replaceReducer(nextReducer: Reducer<S, A>) {
        store.replaceReducer(wrapReducer(nextReducer))
      }
    }
  }

  const store = createStore(reducer, enhancer)

  store.getState().someField
  store.getState().extraField
  // typings:expect-error
  store.getState().wrongField
}

/**
 * Store enhancer that adds methods to the store.
 */
function extraMethods() {
  const enhancer: StoreEnhancer<{ method(): string }> = createStore => (
    ...args
  ) => {
    const store = createStore(...args)
    return {

Review comment (bug):
The change in types actually caught a bug in one the tests where we weren't re-wrapping the reducer previously.

Code after:
function stateExtension() {
  interface ExtraState {
    extraField: 'extra'
  }

  const enhancer: StoreEnhancer<{}, ExtraState> =
    createStore =>
    <S, A extends Action = AnyAction>(
      reducer: Reducer<S, A>,
      preloadedState?: any
    ) => {
      const wrappedReducer: Reducer<S & ExtraState, A> = (state, action) => {
        const newState = reducer(state, action)
        return {
          ...newState,
          extraField: 'extra'
        }
      }
      const wrappedPreloadedState = preloadedState
        ? {
            ...preloadedState,
            extraField: 'extra'
          }
        : undefined
      return createStore(wrappedReducer, wrappedPreloadedState)
    }

  const store = createStore(reducer, enhancer)

  store.getState().someField
  store.getState().extraField
  // @ts-expect-error
  store.getState().wrongField
}

/**
 * Store enhancer that adds methods to the store.
 */
function extraMethods() {
  const enhancer: StoreEnhancer<{ method(): string }> =
    createStore =>
    (...args) => {
      const store = createStore(...args)
      store.method = () => 'foo'
      return store
    }

  const store = createStore(reducer, enhancer)

  store.getState()
  const res: string = store.method()
Repository: reduxjs/redux
File: test/typescript/enhancers.ts (TypeScript)

Code before:
import { StoreEnhancer, Action, AnyAction, Reducer, createStore } from '../..'

interface State {
  someField: 'string'
}
const reducer: Reducer<State> = null as any

/**
 * Store enhancer that extends the type of dispatch.
 */
function dispatchExtension() {
  type PromiseDispatch = <T extends Action>(promise: Promise<T>) => Promise<T>

  const enhancer: StoreEnhancer<{
    dispatch: PromiseDispatch
  }> = createStore => (reducer, preloadedState) => {
    const store = createStore(reducer, preloadedState)

    function replaceReducer<NewState, NewActions extends Action>(
      nextReducer: Reducer<NewState, NewActions>
    ) {
      store.replaceReducer(nextReducer)
      return newStore
    }

    const newStore = {
      ...store,
      dispatch: (action: any) => {
        if (action.type) {
          store.dispatch(action)
        } else if (action.then) {
          action.then(store.dispatch)
        }
        return action
      },
      replaceReducer
    }
    return newStore
  }

  const store = createStore(reducer, enhancer)

  store.dispatch({ type: 'INCREMENT' })
  store.dispatch(Promise.resolve({ type: 'INCREMENT' }))
  // typings:expect-error
  store.dispatch('not-an-action')
  // typings:expect-error
  store.dispatch(Promise.resolve('not-an-action'))
}

Review comment (bug):
Many of the enhancer tests had incorrect `replaceReducer` methods. Previously they were actually returning the old store without the enhancer extensions applied instead of returning the new store with the enhancers applied. The fix in the types caught this bug.

Code after:
import {
  StoreEnhancer,
  Action,
  AnyAction,
  Reducer,
  createStore,
  Store
} from '../../src'

interface State {
  someField: 'string'
}
const reducer: Reducer<State> = null as any

/**
 * Store enhancer that extends the type of dispatch.
 */
function dispatchExtension() {
  type PromiseDispatch = <T extends Action>(promise: Promise<T>) => Promise<T>

  const enhancer: StoreEnhancer<{
    dispatch: PromiseDispatch
  }> =
    createStore =>
    <S, A extends Action = AnyAction>(
      reducer: Reducer<S, A>,
      preloadedState?: any
    ) => {
      const store = createStore(reducer, preloadedState)
      return {
        ...store,
        dispatch: (action: any) => {
          if (action.type) {
            store.dispatch(action)
          } else if (action.then) {
            action.then(store.dispatch)
          }
          return action
        }
      }
    }

  const store = createStore(reducer, enhancer)

  store.dispatch({ type: 'INCREMENT' })
  store.dispatch(Promise.resolve({ type: 'INCREMENT' }))
  // @ts-expect-error
  store.dispatch('not-an-action')
  // @ts-expect-error
Repository: reduxjs/redux
File: src/createStore.ts (TypeScript)

Code before:
      )
    }

    return enhancer(createStore)(
      reducer,
      preloadedState as PreloadedState<S>
    ) as Store<ExtendState<S, StateExt>, A, StateExt, Ext> & Ext
  }

  if (typeof reducer !== 'function') {
    throw new Error(
      `Expected the root reducer to be a function. Instead, received: '${kindOf(
        reducer
      )}'`
    )
  }

  let currentReducer = reducer
  let currentState = preloadedState as S
  let currentListeners: (() => void)[] | null = []
  let nextListeners = currentListeners
  let isDispatching = false

  /**
   * This makes a shallow copy of currentListeners so can use
   * nextListeners as a temporary list while dispatching.
   *
   * This prevents any bugs around consumers calling
   * subscribe/unsubscribe in the middle of a dispatch.
   */
  function ensureCanMutateNextListeners() {
    if (nextListeners === currentListeners) {
      nextListeners = currentListeners.slice()
    }
  }

  /**
   * Reads the state tree managed by the store.
   *
   * @returns The current state tree of your application.
   */
  function getState(): S {
    if (isDispatching) {
      throw new Error(
        'You may not call store.getState() while the reducer is executing. ' +
          'The reducer has already received the state as an argument. ' +
          'Pass it down from the top reducer instead of reading it from the store.'
      )
    }

    return currentState as S

Review comment (suggestion):
```suggestion
   * This makes a shallow copy of currentListeners so nextListeners
   * can be used as a temporary list while dispatching.
```

Code after:
      )
    }

    return enhancer(createStore)(
      reducer,
      preloadedState as PreloadedState<S>
    ) as Store<ExtendState<S, StateExt>, A, StateExt, Ext> & Ext
  }

  if (typeof reducer !== 'function') {
    throw new Error(
      `Expected the root reducer to be a function. Instead, received: '${kindOf(
        reducer
      )}'`
    )
  }

  let currentReducer = reducer
  let currentState = preloadedState as S
  let currentListeners: (() => void)[] | null = []
  let nextListeners = currentListeners
  let isDispatching = false

  /**
   * This makes a shallow copy of currentListeners so we can use
   * nextListeners as a temporary list while dispatching.
   *
   * This prevents any bugs around consumers calling
   * subscribe/unsubscribe in the middle of a dispatch.
   */
  function ensureCanMutateNextListeners() {
    if (nextListeners === currentListeners) {
      nextListeners = currentListeners.slice()
    }
  }

  /**
   * Reads the state tree managed by the store.
   *
   * @returns The current state tree of your application.
   */
  function getState(): S {
    if (isDispatching) {
      throw new Error(
        'You may not call store.getState() while the reducer is executing. ' +
          'The reducer has already received the state as an argument. ' +
          'Pass it down from the top reducer instead of reading it from the store.'
      )
    }

    return currentState as S