-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(clone): enhance type and logic. #206
base: main
Are you sure you want to change the base?
Conversation
Current `clone()` function considers only those native classes. - `Date` - `Set` - `Map` - `RegExp` However, there are much more native classes in the JavaScript, especially about binary handling. So, I think that the `clone()` function should consider them. - `Uint8Array` - `Uint8ClampedArray` - `Uint16Array` - `BigInt64Array` - `Int8Array` - `Int16Array` - `Int32Array` - `BigInt64Array` - `Float32Array` - `Float64Array` - `ArrayBuffer` - `SharedArrayBuffer` - `DataView` - `Blob` - `File` Also, current `clone()` function is returning the same `T` type with its parameter, but it is not correct. The returned type must be casted, because non-native classes are converted to primitve object type. To solve this problem, the `clone()` function needs to return `Shallowed<T>` type like below. ```typescript type Shallowed<T> = Equal<T, ShallowMain<T>> extends true ? T : ShallowMain<T> type ShallowMain<T> = T extends [never] ? never : T extends object ? T extends | Array<any> | Set<any> | Map<any, any> | Date | RegExp | Date | Uint8Array | Uint8ClampedArray | Uint16Array | Uint32Array | BigUint64Array | Int8Array | Int16Array | Int32Array | BigInt64Array | Float32Array | Float64Array | ArrayBuffer | SharedArrayBuffer | DataView | Blob | File ? T : { [P in keyof T]: T[P] extends Function ? never : T[P]; } : T; type Equal<X, Y> = X extends Y ? (Y extends X ? true : false) : false; ``` - related issue: toss#196 - related PR: toss#155
@samchon is attempting to deploy a commit to the Toss Team on Vercel. A member of the Team first needs to authorize it. |
It's a very complex type, but that doesn't mean it's overly pedantic. This is because this type is more like solving an error unlike the previous one. In most libraries, there was a problem that the clone type did not properly respond to the class type. |
src/object/clone.spec.ts
Outdated
// check whether two types are equal only in the compiler level | ||
let x: Shallowed<SomeClass> = null!; | ||
let y: SomeInterface = null!; | ||
x = y; | ||
y = x; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this has to be tested with some other type testing library.
See vitest's docs for details.
: OmitNever<{ | ||
[P in keyof T]: T[P] extends Function ? never : T[P]; | ||
}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this type does here. Could you explain what this is doing?
I thought cloning some objects with methods, like clone({ foo: () => {} })
will also work. However this type definition does not seem to respect it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be many edge cases, so we might just return T
from clone
. After all, it's well-known that shallow cloning class instances doesn't copy methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A developer working on the code might know if a value has been shallow copied, but other collaborators may not be aware without inspecting the code (specifically, the function returning the cloned value). Therefore, I believe the type proposed by samchon can help reduce misunderstandings among developers.
Using this type enhances code readability and prevents unnecessary bugs and confusion within the team. By employing the Resolved type, we can avoid the misconception of certain methods existing on plain objects, ensuring that developers understand the correct nature of the cloned value and can use the code more accurately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that clone
does not clone class instance's methods.
I was wondering what will happen to clone({ foo: () => {} })
; Here, we can correctly clone the foo
property. However it will be prohibited by types.
As far as I know, we cannot easily distinguish class methods and function properties, since TypeScript uses structural typing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that
clone
does not clone class instance's methods.I was wondering what will happen to
clone({ foo: () => {} })
; Here, we can correctly clone thefoo
property. However it will be prohibited by types.As far as I know, we cannot easily distinguish class methods and function properties, since TypeScript uses structural typing.
To fix the problem, changed the implementation code.
return Object.fromEntries(
Object.entries(obj).filter(([_key, value]) => value !== undefined && typeof value !== "function")
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think changing the implementation is awkward since most people would expect that foo
will be properly cloned. Note that lodash
will clone those properties. In JavaScript function properties are cloned when shallow copying an object.
const object = { foo: () => {} };
const cloned = { ...object };
I think types should follow the implementation. Implementation should not follow the types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, unless there is a TypeScript type that simply handles this case, we allow this behavior, because I think cloning a class instance is not a major use case.
Please refer to our contributing guide where we value simple implementation, and our functions will have complex implementation to suit every usecase.
Co-authored-by: novo <63547292+de-novo@users.noreply.github.com>
Current
clone()
function considers only those native classes.Date
Set
Map
RegExp
However, there are much more native classes in the JavaScript, especially about binary handling.
So, I think that the
clone()
function should consider them.Uint8Array
Uint8ClampedArray
Uint16Array
BigInt64Array
Int8Array
Int16Array
Int32Array
BigInt64Array
Float32Array
Float64Array
ArrayBuffer
SharedArrayBuffer
DataView
Blob
File
Also, current
clone()
function is returning the sameT
type with its parameter, but it is not correct.The returned type must be casted, because non-native classes are converted to primitve object type.
To solve this problem, the
clone()
function needs to returnShallowed<T>
type like below.clone()
function. #196