es-toolkit icon indicating copy to clipboard operation
es-toolkit copied to clipboard

Proposal about type constraints in object utils

Open mass2527 opened this issue 1 year ago • 1 comments

Overview

Currently, in the object utilities, many functions, including pick, constrain the obj parameter as Record<string, any>. This type constraint is too broad and allows non-plain objects to be passed in, which could lead to unintended behavior.

Example: pick Function

The pick function is a good example of this issue:

function pick<T extends Record<string, any>, K extends keyof T>(obj: T, keys: K[]): Pick<T, K> {
  const result = {} as Pick<T, K>;

  for (const key of keys) {
    result[key] = obj[key];
  }

  return result;
}

Issue with Record<string, any>

Using Record<string, any> allows any non-primitive values to be passed:

type Value = Record<string, any>;

// primitive types (string, number, boolean, undefined, null, symbol, bigint) cannot be assigned
// const value: Value = '' // error
// const value: Value = 0 // error
// const value: Value = true // error
// const value: Value = undefined // error
// const value: Value = null // error
// const value: Value = Symbol() // error
// const value: Value = 0n // error

// object types can be assigned
// const value: Value = () => {} // okay
// const value: Value = {} // okay
// const value: Value = [] // okay
// const value: Value = new Date() // okay
// and more...

This means that users can call these utility functions with non-plain objects, leading to cases like this:

pick(['a','b','c'], []) // no error
pick(() => {}, []) // no error
pick(new Date(), []) // no error

Proposal: Restrict to Record<string, unknown>

Although passing non-plain objects like arrays is possible, I believe it is not a common use case. Given the ES Toolkit’s design philosophy of simplicity and focusing on the most common 85% of use cases, I propose restricting the type constraint to Record<string, unknown>, which only allows plain objects:

type Value = Record<string, unknown>;

// const value: Value = () => {} // error
// const value: Value = {} // okay
// const value: Value = [] // error
// const value: Value = new Date() // error

Conclusion

Restricting the type constraint in object utilities to Record<string, unknown> would prevent non-plain objects from being passed to these functions, aligning with the toolkit’s goal of simplicity.

mass2527 avatar Aug 17 '24 06:08 mass2527

interface ESToolkit {
  code: string
}

class ES {
  constructor(code: string) {
    this.code = code
  }
}

const es_toolkit: ESToolkit = {code: "Hello"}
const es = new ES("Hello") 


const obj: Record<string, unknown> = es_toolkit  // error
const obj2: Record<string, unknown> = es // error

If we use a Record<string, unknown>, then we can't use with interface and instance, so I think that maybe using Record<string, unknown> makes unexpected behavior. Thanks!

https://www.typescriptlang.org/play/?#code/JYOwLgpgTgZghgYwgAgKIGUAqB7bAbAa2DGQG8AoZZBbAEwgC5kBnMKUAc3IF9zyE8cZszToylathCsoAVwRhsUABQ16TGZwCU4qlTAALYMwB0alAF5J9Cb178prZBGYB9RfiJgmGHJ+LIVqTmTABEyAASEHh42KG8NNIkLoHIIBAA7qLKoVExcVp8DknI2ABGAFZMAEoQNFC0ADyaIBwANMiyIAQg2BkgAHypLu64hMTFTuUVAEw1dUpNLe2d3b39Q1YuQA

dayongkr avatar Aug 19 '24 01:08 dayongkr

@dayongkr Thanks for your explanation!

I agree with you—while Record<string, unknown> can narrow the type, the cons you mentioned outweigh the benefits. I think it’s better to stick with the current implementation.

mass2527 avatar Aug 20 '24 13:08 mass2527