Skip to content

refactor: replace custom cloneDeep with structuredClone - #10933

Open
VishalRaut2106 wants to merge 2 commits into
firebase:mainfrom
VishalRaut2106:refactor-clone-deep
Open

refactor: replace custom cloneDeep with structuredClone#10933
VishalRaut2106 wants to merge 2 commits into
firebase:mainfrom
VishalRaut2106:refactor-clone-deep

Conversation

@VishalRaut2106

Copy link
Copy Markdown
Contributor

refactor: replace custom cloneDeep with structuredClone

Description

Replaced the custom recursive cloneDeep implementation in src/utils.ts with Node.js' native structuredClone(), since the CLI's minimum Node.js requirement is now v18+. This removes technical debt and improves cloning performance. Includes a fallback to handle edge-cases where objects have functions (which aren't serializable).

Scenarios Tested

Ran Mocha tests against utilities utilizing cloneDeep (e.g. src/requireConfig.spec.ts) to ensure object references are properly broken and clones act independently without triggering DataCloneError.

Sample Commands

N/A

### Description
Replaced the custom recursive cloneDeep implementation in src/utils.ts with Node.js native structuredClone(), since the CLI minimum Node.js requirement is now v18+. This removes technical debt and improves cloning performance. Includes a fallback to handle edge-cases where objects have functions (which are not serializable).

### Scenarios Tested
Ran Mocha tests against utilities utilizing cloneDeep (e.g. src/requireConfig.spec.ts) to ensure object references are properly broken and clones act independently without triggering DataCloneError.

### Sample Commands
N/A

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request simplifies the cloneDeep utility in src/utils.ts by replacing the custom deep cloning logic with the native structuredClone API. However, the new implementation introduces a correctness regression where non-serializable objects (such as those containing functions) silently return the original reference upon failure, leading to unexpected mutations. Additionally, removing the fast-path checks for primitives and functions causes a performance regression. It is recommended to restore the fast-path checks and fall back to _.cloneDeep if structuredClone fails.

Comment thread src/utils.ts
Comment on lines 631 to 640
export function cloneDeep<T>(obj: T): T {
if (typeof obj !== "object" || !obj) {
if (obj === undefined) {
return obj;
}
if (obj instanceof RegExp) {
return RegExp(obj, obj.flags) as typeof obj;
}
if (obj instanceof Date) {
return new Date(obj) as typeof obj;
}
if (Array.isArray(obj)) {
return cloneArray(obj) as typeof obj;
}
if (obj instanceof Map) {
return new Map(obj.entries()) as typeof obj;
try {
return structuredClone(obj);
} catch (e) {
return obj;
}
return cloneObject(obj as Record<string, unknown>) as typeof obj;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The new implementation introduces two significant issues:

  1. Correctness Regression (High Severity): If structuredClone throws an error (e.g., when the object contains a function, symbol, or other non-serializable property), the catch block silently returns the original object reference obj. This is a regression from the original cloneDeep which recursively cloned the object structure and copied functions by reference, ensuring the outer object reference was broken. Returning the original reference means any subsequent mutations to the "cloned" object will unexpectedly mutate the original object, leading to silent and hard-to-debug bugs.
  2. Performance Regression (Medium Severity): The fast-path check for primitives, null, and functions has been removed. Only checking obj === undefined means all other primitives (like strings, numbers, booleans, null) and functions will go through the try/catch block and call structuredClone(), which is much slower than a simple type check.

We can resolve both issues by restoring the fast-path check and falling back to _.cloneDeep (which is already imported in this file from lodash) when structuredClone fails.

Suggested change
export function cloneDeep<T>(obj: T): T {
if (typeof obj !== "object" || !obj) {
if (obj === undefined) {
return obj;
}
if (obj instanceof RegExp) {
return RegExp(obj, obj.flags) as typeof obj;
}
if (obj instanceof Date) {
return new Date(obj) as typeof obj;
}
if (Array.isArray(obj)) {
return cloneArray(obj) as typeof obj;
}
if (obj instanceof Map) {
return new Map(obj.entries()) as typeof obj;
try {
return structuredClone(obj);
} catch (e) {
return obj;
}
return cloneObject(obj as Record<string, unknown>) as typeof obj;
}
export function cloneDeep<T>(obj: T): T {
if (typeof obj !== "object" || !obj) {
return obj;
}
try {
return structuredClone(obj);
} catch (e) {
return _.cloneDeep(obj);
}
}
References
  1. Reduce nesting as much as possible: Code should avoid unnecessarily deep nesting or long periods of nesting. Use early returns, continue, and break statements in functions and loops to handle edge cases early and keep main logic flat. (link)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants