ronantakizawa/codereview-bench huggingface dataset:
Repository: pmndrs/zustand
File: tests/basic.test.tsx (TypeScript)

Code before:

  // ok, should not error
  store.setState((previous) => {
    if (previous.count === 0) {
      return { count: 1 }
    }
    return { count: 0 }
  })
  store.setState<'count', 'name'>((previous) => {
    if (previous.count === 0) {
      return { count: 1 }
    }
    if (previous.count === 1) {
      return previous
    }
    return { name: 'foo' }
  })

  // @ts-expect-error type  Type '{ name: boolean; count?: undefined; }' is not assignable to type State.
  store.setState<'count', 'name'>((previous) => {
    if (previous.count === 0) {
      return { count: 1 }
    }
    return { name: true }
  })
})


Review comment (bug):
i would expect an error here, because `name: true` certainly does not fit. However, there is no error, and I don't really know why. it errors fine the [ts playground](https://www.typescriptlang.org/play?ts=4.2.3#code/KYDwDg9gTgLgBDAnmYcDKMCGNUF44QBGAVsAMYwBQA9NQssAM5wCWAdo2C1MACZyFEcRsBgZsqAGZQIAWzgAlYJgoAaYcGAAuGnQAWMGGEZbaAcxYw9AV0IA6MnOoARYJPaXgAG0QAVBrwubh44Pv4ogYReRNQAbGQALACcwAkArAAcCQBMAMyZhJhpSUUADBnAAIy51aUA7EmVSRlJjZUJuaWBSCiM1DwqMNQAbpWx1Oy8oHa8djCMAMQAMgktALQrSWmUoJCw9ChwAAqYsCyYXuI4ADy+cKA4bLzMV8DqANKV9yCPz3AA1sBEBBJHA7vhAcDQb4Ptlvr9mJCQWC4PhPh9cvDgE9EUDkeC4O9sh8ElicQC8dDUYTcgA+VGUOBwAA+cAAFEcWGR-rcPpV6azOdzeYTsgLjlyeTCaeKhVKSeLfABKRks9lsxhYHBaMFK1H0uUiz6yyVGsVqw3S950i2mq0JRUq3bQeA9VCvNDecgwaC3Ml-V7qACq9PwGq12l1+rgQZ24BdB1QAFEAI7WC6WRAAYT05EBUFuofZmokOulbGAAHdXmW9bh6YQIBAvMo2HG9q6GOgI0sWJrscAC74i+HS2D1GAeMMWBBrIwa1H63BhhAWLx2wm3d2JGgvFzgL3+xWhyPGHuyJHpZPgNPZ-Pz5e6-SV2uN-t2DgoJIVO7bIwyFALCEMAfoPNiAYRvSADeqpsnuR6DjqryHo8g6FkqOpsk+y6ruuTLXB6D60myqpMvBqFQEhEa7vuKEDgWhH7rSqikRoLYUNAAD8VE7l6HFDuojEXsxrHAGmGZIAAYmw3FwKm6Z7kgOZ5mhQnALSqoYey2EvuuAC+lBvp2hyemIEagT84EvJB1Iwfhnz+riUIohClLjqKjkUs5BLojSnlIlSaLEoSpJgeSAUuTKbJgKcMDnF4OonGcFyvEalSwhiCrqDwYBeD+smNs2rZabplAGc6+xbgA4qIqV3GFEESCO2G+EZiZwK4moyEIYY6bhbUfoO34Xt20DAAAglwFkItuODQaqIhmWOpl1RpTJmLVEY6jVS03MOC1-gBQGRmgh2AcBhaqlMXUQIgOqdTA3VlW1W6vFmAw+kOnmBnAWZzj6sgrRG1JAxIhYjgtog6n9mpyKDOAsetUNwDtq2I3AmBcFRY2TSwl0tYZlCOBw8Cwzw1IAES8NYsiyIgFMY8w1hsP8bAQJWbCM6NPC49cUFwJITY6l17BmAA3AIpw6mwNPAVAcB6RpZPAHYi2pQA5ILEDq+o6uFFA6vEdFU4znO2F2awoLGzepuMHY+uorg+CVHqFtMjwMDWFAnP81rOoU1rDN6WLqoGe7ohez7kuUQgUDWKgwdlUqIeUEAA)

@dai-shi do you see why?

Code after:

  // ok, should not error
  store.setState((previous) => {
    if (previous.count === 0) {
      return { count: 1 }
    }
    return { count: 0 }
  })
  store.setState<'count', 'something'>((previous) => {
    if (previous.count === 0) {
      return { count: 1 }
    }
    if (previous.count === 1) {
      return previous
    }
    return { something: 'foo' }
  })

  // @ts-expect-error Type '{ something: boolean; count?: undefined; }' is not assignable to type 'State'.
  store.setState<'count', 'something'>((previous) => {
    if (previous.count === 0) {
      return { count: 1 }
    }
    return { something: true }
  })
})
Repository: pmndrs/zustand
File: src/middleware.ts (TypeScript)

Code before:
        console.warn(
          `Persist middleware: unable to update ${name}, the given storage is currently unavailable.`
        )
        set(...args)
      },
      get,
      api
    )
  }

  const setItem = (): Promise<void> | void => {
    const state = { ...get() }

    if (whitelist) {
      ;(Object.keys(state) as (keyof S)[]).forEach((key) => {
        !whitelist.includes(key) && delete state[key]
      })
    }
    if (blacklist) {
      blacklist.forEach((key) => delete state[key])
    }

    let serializeResult = serialize({ state, version })
    return serializeResult instanceof Promise
      ? serializeResult.then((value) => storage?.setItem(name, value))
      : storage?.setItem(name, serializeResult)
  }

  const savedSetState = api.setState

  api.setState = (state, replace) => {
    savedSetState(state, replace)
    void setItem()
  }

  // rehydrate initial state with existing stored state
  ;(() => {
    const postRehydrationCallback = onRehydrateStorage?.(get()) || undefined

    let storageValueUnknown = storage.getItem(name)

    if (storageValueUnknown instanceof Promise) {
      storageValueUnknown
        .then((storageValue) => storageValue && deserialize(storageValue))
        .then((deserializedStorageValue) => {
          if (
            deserializedStorageValue &&
            deserializedStorageValue.version !== version
          ) {
            return migrate?.(
              deserializedStorageValue.state,

Review comment (suggestion):
To keep the behavior, we don't invoke serialize, if storage is undefined.
```suggestion
    if (storage) {
      const serializeResult = serialize({ state, version })
      return serializeResult instanceof Promise
        ? serializeResult.then((value) => storage.setItem(name, value))
        : storage.setItem(name, serializeResult)
    }
```

Code after:
        return this as Thenable<any>
      },
    }
  } catch (e) {
    return {
      then(_onFulfilled) {
        return this as Thenable<any>
      },
      catch(onRejected) {
        return toThenable(onRejected)(e)
      },
    }
  }
}

export const persist = <S extends State>(
  config: StateCreator<S>,
  options: PersistOptions<S>
) => (set: SetState<S>, get: GetState<S>, api: StoreApi<S>): S => {
  const {
    name,
    getStorage = () => localStorage,
    serialize = JSON.stringify as (state: StorageValue<S>) => string,
    deserialize = JSON.parse as (str: string) => StorageValue<S>,
    blacklist,
    whitelist,
    onRehydrateStorage,
    version = 0,
    migrate,
  } = options || {}

  let storage: StateStorage | undefined

  try {
    storage = getStorage()
  } catch (e) {
    // prevent error if the storage is not defined (e.g. when server side rendering a page)
  }

  if (!storage) {
    return config(
      (...args) => {
        console.warn(
          `Persist middleware: unable to update ${name}, the given storage is currently unavailable.`
        )
        set(...args)
      },
      get,
      api
    )
  }
Repository: pmndrs/zustand
File: src/middleware.ts (TypeScript)

Code before:
    if (blacklist) {
      blacklist.forEach((key) => delete state[key])
    }

    let serializeResult = serialize({ state, version })
    return serializeResult instanceof Promise
      ? serializeResult.then((value) => storage?.setItem(name, value))
      : storage?.setItem(name, serializeResult)
  }

  const savedSetState = api.setState

  api.setState = (state, replace) => {
    savedSetState(state, replace)
    void setItem()
  }

  // rehydrate initial state with existing stored state
  ;(() => {
    const postRehydrationCallback = onRehydrateStorage?.(get()) || undefined

    let storageValueUnknown = storage.getItem(name)

    if (storageValueUnknown instanceof Promise) {
      storageValueUnknown
        .then((storageValue) => storageValue && deserialize(storageValue))
        .then((deserializedStorageValue) => {
          if (
            deserializedStorageValue &&
            deserializedStorageValue.version !== version
          ) {
            return migrate?.(
              deserializedStorageValue.state,
              deserializedStorageValue.version
            )
          } else {
            set(deserializedStorageValue.state)
            return null
          }
        })
        .then((migratedState) => {
          if (migratedState) {
            set(migratedState)
            return setItem()
          }
        })
        .then(() => {
          postRehydrationCallback?.(get(), undefined)
        })
        .catch((e) => {
          postRehydrationCallback?.(undefined, e)

Review comment (suggestion):
The original code doesn't seem to check `storageValue`.
```suggestion
        .then((storageValue) => deserialize(storageValue))
```

Code after:
) => (set: SetState<S>, get: GetState<S>, api: StoreApi<S>): S => {
  const {
    name,
    getStorage = () => localStorage,
    serialize = JSON.stringify as (state: StorageValue<S>) => string,
    deserialize = JSON.parse as (str: string) => StorageValue<S>,
    blacklist,
    whitelist,
    onRehydrateStorage,
    version = 0,
    migrate,
  } = options || {}

  let storage: StateStorage | undefined

  try {
    storage = getStorage()
  } catch (e) {
    // prevent error if the storage is not defined (e.g. when server side rendering a page)
  }

  if (!storage) {
    return config(
      (...args) => {
        console.warn(
          `Persist middleware: unable to update ${name}, the given storage is currently unavailable.`
        )
        set(...args)
      },
      get,
      api
    )
  }

  const thenableSerialize = toThenable(serialize)

  const setItem = (): Thenable<void> => {
    const state = { ...get() }

    if (whitelist) {
      ;(Object.keys(state) as (keyof S)[]).forEach((key) => {
        !whitelist.includes(key) && delete state[key]
      })
    }
    if (blacklist) {
      blacklist.forEach((key) => delete state[key])
    }

    let errorInSync: Error | undefined
    const thenable = thenableSerialize({ state, version })
      .then((serializedValue) =>
Repository: pmndrs/zustand
File: src/middleware.ts (TypeScript)

Code before:
                deserializedStorageValue.state,
                deserializedStorageValue.version
              )
            )
          } else {
            set(deserializedStorageValue.state)
            return makeThenable(null)
          }
        } else {
          return makeThenable(null)
        }
      })
      .then((migratedState) => {
        if (migratedState) {
          set(migratedState)
          return makeThenable(setItem())
        } else {
          return makeThenable(null)
        }
      })
      .then(() => {
        postRehydrationCallback?.(get(), undefined)
        return makeThenable(null)
      })
      // this handler will only catch promise related errors
      .catch((e) => {
        postRehydrationCallback?.(undefined, e)
      })
  })()

  return config(
    (...args) => {
      set(...args)
      void setItem()
    },
    get,
    api
  )
}


Review comment (suggestion):
This `catch` handles only errors thrown in async mode. For sync mode the old `try...catch` needs to be reintroduced, meaning we need both `catch()` and `try...catch`.

Code after:
    let errorInSync: Error | undefined
    const thenable = thenableSerialize({ state, version })
      .then((serializedValue) =>
        (storage as StateStorage).setItem(name, serializedValue)
      )
      .catch((e) => {
        errorInSync = e
      })
    if (errorInSync) {
      throw errorInSync
    }
    return thenable
  }

  const savedSetState = api.setState

  api.setState = (state, replace) => {
    savedSetState(state, replace)
    void setItem()
  }

  // rehydrate initial state with existing stored state

  // a workaround to solve the issue of not storing rehydrated state in sync storage
  // the set(state) value would be later overridden with initial state by create()
  // to avoid this, we merge the state from localStorage into the initial state.
  let stateFromStorageInSync: S | undefined
  const postRehydrationCallback = onRehydrateStorage?.(get()) || undefined
  // bind is used to avoid `TypeError: Illegal invocation` error
  toThenable(storage.getItem.bind(storage))(name)
    .then((storageValue) => {
      if (storageValue) {
        return deserialize(storageValue)
      }
    })
    .then((deserializedStorageValue) => {
      if (deserializedStorageValue) {
        if (deserializedStorageValue.version !== version) {
          if (migrate) {
            return migrate(
              deserializedStorageValue.state,
              deserializedStorageValue.version
            )
          }
          console.error(
            `State loaded from storage couldn't be migrated since no migrate function was provided`
          )
        } else {
          stateFromStorageInSync = deserializedStorageValue.state
          set(deserializedStorageValue.state)
        }
Repository: pmndrs/zustand
File: src/middleware.ts (TypeScript)

Code before:
    }
    if (blacklist) {
      blacklist.forEach((key) => delete state[key])
    }

    if (storage) {
      const serializeResult = serialize({ state, version })
      return serializeResult instanceof Promise
        ? serializeResult.then((value) => storage.setItem(name, value))
        : storage.setItem(name, serializeResult)
    }
  }

  const savedSetState = api.setState

  api.setState = (state, replace) => {
    savedSetState(state, replace)
    void setItem()
  }

  const makeThenable = (value: any) => {
    if (value instanceof Promise) {
      return value
    } else {
      return {
        then: (callback) => callback(value),
        // this is a fake function just to avoid catch is not a function error
        catch: () => undefined,
      }
    }
  }
  // rehydrate initial state with existing stored state
  ;(() => {
    const postRehydrationCallback = onRehydrateStorage?.(get()) || undefined

    let storageValuePending = makeThenable(storage.getItem(name))
    storageValuePending
      .then((storageValue) => {
        if (storageValue) {
          return makeThenable(deserialize(storageValue))
        } else {
          return makeThenable(null)
        }
      })
      .then((deserializedStorageValue) => {
        if (deserializedStorageValue) {
          if (deserializedStorageValue.version !== version) {
            return makeThenable(
              migrate?.(
                deserializedStorageValue.state,
                deserializedStorageValue.version

Review comment (bug):
```suggestion
        then: (callback) => makeThenable(callback(value)),
```
Not strictly necessary, but maybe a good idea. I had a bug where I forgot to manually wrap the result of `then()` with `makeThenable()`. Also it would resemble real promises a more.

Code after:
  options: PersistOptions<S>
) => (set: SetState<S>, get: GetState<S>, api: StoreApi<S>): S => {
  const {
    name,
    getStorage = () => localStorage,
    serialize = JSON.stringify as (state: StorageValue<S>) => string,
    deserialize = JSON.parse as (str: string) => StorageValue<S>,
    blacklist,
    whitelist,
    onRehydrateStorage,
    version = 0,
    migrate,
  } = options || {}

  let storage: StateStorage | undefined

  try {
    storage = getStorage()
  } catch (e) {
    // prevent error if the storage is not defined (e.g. when server side rendering a page)
  }

  if (!storage) {
    return config(
      (...args) => {
        console.warn(
          `Persist middleware: unable to update ${name}, the given storage is currently unavailable.`
        )
        set(...args)
      },
      get,
      api
    )
  }

  const thenableSerialize = toThenable(serialize)

  const setItem = (): Thenable<void> => {
    const state = { ...get() }

    if (whitelist) {
      ;(Object.keys(state) as (keyof S)[]).forEach((key) => {
        !whitelist.includes(key) && delete state[key]
      })
    }
    if (blacklist) {
      blacklist.forEach((key) => delete state[key])
    }

    let errorInSync: Error | undefined
    const thenable = thenableSerialize({ state, version })
Repository: pmndrs/zustand
File: src/middleware.ts (TypeScript)

Code before:

    if (whitelist) {
      ;(Object.keys(state) as (keyof S)[]).forEach((key) => {
        !whitelist.includes(key) && delete state[key]
      })
    }
    if (blacklist) {
      blacklist.forEach((key) => delete state[key])
    }

    if (storage) {
      const serializeResult = serialize({ state, version })
      return serializeResult instanceof Promise
        ? serializeResult.then((value) => storage.setItem(name, value))
        : storage.setItem(name, serializeResult)
    }
  }

  const savedSetState = api.setState

  api.setState = (state, replace) => {
    savedSetState(state, replace)
    void setItem()
  }

  const makeThenable = (value: any) => {
    if (value instanceof Promise) {
      return value
    } else {
      return {
        then: (callback) => callback(value),
        // this is a fake function just to avoid catch is not a function error
        catch: () => undefined,
      }
    }
  }
  // rehydrate initial state with existing stored state
  ;(() => {
    const postRehydrationCallback = onRehydrateStorage?.(get()) || undefined

    let storageValuePending = makeThenable(storage.getItem(name))
    storageValuePending
      .then((storageValue) => {
        if (storageValue) {
          return makeThenable(deserialize(storageValue))
        } else {
          return makeThenable(null)
        }
      })
      .then((deserializedStorageValue) => {
        if (deserializedStorageValue) {

Review comment (suggestion):
Does it work for error handling.
It has to be something like this? not 100% sure.
```ts
type Thenable<Result> = {
  then: (onFulfilled: ..., onRejected: ...) => ...
}
const makeFunctionThatReturnsThenable = <Input, Result>(
  fn: ((input: Input) => Result | Promise<Result>)
): (input: Input) => Thenable<Result> => {
 ...
}
```
Also, we might want to make types correctly to avoid misunderstanding.


Code after:
  }
}

export const persist = <S extends State>(
  config: StateCreator<S>,
  options: PersistOptions<S>
) => (set: SetState<S>, get: GetState<S>, api: StoreApi<S>): S => {
  const {
    name,
    getStorage = () => localStorage,
    serialize = JSON.stringify as (state: StorageValue<S>) => string,
    deserialize = JSON.parse as (str: string) => StorageValue<S>,
    blacklist,
    whitelist,
    onRehydrateStorage,
    version = 0,
    migrate,
  } = options || {}

  let storage: StateStorage | undefined

  try {
    storage = getStorage()
  } catch (e) {
    // prevent error if the storage is not defined (e.g. when server side rendering a page)
  }

  if (!storage) {
    return config(
      (...args) => {
        console.warn(
          `Persist middleware: unable to update ${name}, the given storage is currently unavailable.`
        )
        set(...args)
      },
      get,
      api
    )
  }

  const thenableSerialize = toThenable(serialize)

  const setItem = (): Thenable<void> => {
    const state = { ...get() }

    if (whitelist) {
      ;(Object.keys(state) as (keyof S)[]).forEach((key) => {
        !whitelist.includes(key) && delete state[key]
      })
    }
    if (blacklist) {
Repository: pmndrs/zustand
File: src/middleware.ts (TypeScript)

Code before:
        }
      })
      .then((migratedState) => {
        if (migratedState) {
          set(migratedState as PartialState<S, keyof S>)
          return setItem()
        }
      })
      .then(() => {
        postRehydrationCallback?.(get(), undefined)
      })
      .catch((e: Error) => {
        postRehydrationCallback?.(undefined, e)
      })
  })()

  const configResult = config(
    (...args) => {
      set(...args)
      void setItem()
    },
    get,
    api
  )

  return { ...configResult, ...(stateFromStorage || {}) }
}


Review comment (suggestion):
This is the workaround, right? I'm not sure if I fully understand it.

```suggestion
  return stateFromStorage ? configResult : { ...configResult, ...stateFromStorage }
```
should be better.

Code after:
    })
    .then((deserializedStorageValue) => {
      if (deserializedStorageValue) {
        if (deserializedStorageValue.version !== version) {
          if (migrate) {
            return migrate(
              deserializedStorageValue.state,
              deserializedStorageValue.version
            )
          }
          console.error(
            `State loaded from storage couldn't be migrated since no migrate function was provided`
          )
        } else {
          stateFromStorageInSync = deserializedStorageValue.state
          set(deserializedStorageValue.state)
        }
      }
    })
    .then((migratedState) => {
      if (migratedState) {
        stateFromStorageInSync = migratedState as S
        set(migratedState as PartialState<S, keyof S>)
        return setItem()
      }
    })
    .then(() => {
      postRehydrationCallback?.(get(), undefined)
    })
    .catch((e: Error) => {
      postRehydrationCallback?.(undefined, e)
    })

  const configResult = config(
    (...args) => {
      set(...args)
      void setItem()
    },
    get,
    api
  )

  return stateFromStorageInSync
    ? { ...configResult, ...stateFromStorageInSync }
    : configResult
}
Repository: pmndrs/zustand
File: src/middleware.ts (TypeScript)

Code before:
    return config(
      (...args) => {
        console.warn(
          `Persist middleware: unable to update ${name}, the given storage is currently unavailable.`
        )
        set(...args)
      },
      get,
      api
    )
  }

  const setItem = (): Promise<void> | void => {
    const state = { ...get() }

    if (whitelist) {
      ;(Object.keys(state) as (keyof S)[]).forEach((key) => {
        !whitelist.includes(key) && delete state[key]
      })
    }
    if (blacklist) {
      blacklist.forEach((key) => delete state[key])
    }

    if (storage) {
      toThenable(serialize)({ state, version }).then((serializedValue) => {
        storage?.setItem(name, serializedValue)
      })
    }
  }

  const savedSetState = api.setState

  api.setState = (state, replace) => {
    savedSetState(state, replace)
    void setItem()
  }

  // rehydrate initial state with existing stored state

  // a workaround to solve the issue of not storing rehydrated state in sync storage
  // the set(state) value would be later overridden with initial state by create()
  // to avoid this, we merge the state from localStorage into the initial state.
  // TODO: find a better solution for this
  let stateFromStorage: any
  const postRehydrationCallback = onRehydrateStorage?.(get()) || undefined
  // bind is used to avoid `TypeError: Illegal invocation` error
  toThenable(storage.getItem.bind(storage))(name)
    .then((storageValue) => {
      if (storageValue) {
        return deserialize(storageValue)

Review comment (suggestion):
```suggestion
      return toThenable(serialize)({ state, version }).then((serializedValue) => {
```
Not sure if the `storage?.setItem` should also be returned since it's possibly async? 🤔 

Code after:
  let storage: StateStorage | undefined

  try {
    storage = getStorage()
  } catch (e) {
    // prevent error if the storage is not defined (e.g. when server side rendering a page)
  }

  if (!storage) {
    return config(
      (...args) => {
        console.warn(
          `Persist middleware: unable to update ${name}, the given storage is currently unavailable.`
        )
        set(...args)
      },
      get,
      api
    )
  }

  const thenableSerialize = toThenable(serialize)

  const setItem = (): Thenable<void> => {
    const state = { ...get() }

    if (whitelist) {
      ;(Object.keys(state) as (keyof S)[]).forEach((key) => {
        !whitelist.includes(key) && delete state[key]
      })
    }
    if (blacklist) {
      blacklist.forEach((key) => delete state[key])
    }

    let errorInSync: Error | undefined
    const thenable = thenableSerialize({ state, version })
      .then((serializedValue) =>
        (storage as StateStorage).setItem(name, serializedValue)
      )
      .catch((e) => {
        errorInSync = e
      })
    if (errorInSync) {
      throw errorInSync
    }
    return thenable
  }

  const savedSetState = api.setState