Skip to content
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

fix(core): make deepCopy backward compatible #4505

Merged
merged 12 commits into from
Nov 2, 2022
Merged
2 changes: 1 addition & 1 deletion packages/nodes-base/nodes/Code/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export function isObject(maybe: unknown): maybe is { [key: string]: unknown } {
}

function isTraversable(maybe: unknown): maybe is IDataObject {
return isObject(maybe) && Object.keys(maybe).length > 0;
return isObject(maybe) && typeof maybe.toJSON !== 'function' && Object.keys(maybe).length > 0;
}

export type CodeNodeMode = 'runOnceForAllItems' | 'runOnceForEachItem';
Expand Down
6 changes: 5 additions & 1 deletion packages/workflow/src/NodeErrors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,12 @@ abstract class NodeError extends ExecutionBaseError {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
protected isTraversableObject(value: any): value is JsonObject {
return (
value &&
typeof value === 'object' &&
!Array.isArray(value) &&
typeof value.toJSON !== 'function' &&
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
value && typeof value === 'object' && !Array.isArray(value) && !!Object.keys(value).length
!!Object.keys(value).length
);
}

Expand Down
34 changes: 19 additions & 15 deletions packages/workflow/src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,34 +1,38 @@
/* eslint-disable @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-return, @typescript-eslint/no-unsafe-argument */
export const deepCopy = <T>(source: T, hash = new WeakMap(), path = ''): T => {
let clone: any;
let i: any;
type Primitives = string | number | boolean | bigint | symbol | null | undefined;
export const deepCopy = <T extends ((object | Date) & { toJSON?: () => string }) | Primitives>(
source: T,
hash = new WeakMap(),
path = '',
): T => {
const hasOwnProp = Object.prototype.hasOwnProperty.bind(source);
// Primitives & Null & Function
if (typeof source !== 'object' || source === null || source instanceof Function) {
if (typeof source !== 'object' || source === null || typeof source === 'function') {
return source;
}
// Date and other objects with toJSON method
// TODO: remove this when other code parts not expecting objects with `.toJSON` method called and add back checking for Date and cloning it properly
if (typeof source.toJSON === 'function') {
return source.toJSON() as T;
}
if (hash.has(source)) {
return hash.get(source);
}
// Date
if (source instanceof Date) {
cstuncsik marked this conversation as resolved.
Show resolved Hide resolved
return new Date(source.getTime()) as T;
}
// Array
if (Array.isArray(source)) {
clone = [];
const clone = [];
const len = source.length;
for (i = 0; i < len; i++) {
clone[i] = deepCopy(source[i], hash, path + `[${i as string}]`);
for (let i = 0; i < len; i++) {
clone[i] = deepCopy(source[i], hash, path + `[${i}]`);
}
return clone;
return clone as T;
}
// Object
clone = {};
const clone = Object.create(Object.getPrototypeOf({}));
Copy link
Member Author

@netroy netroy Nov 2, 2022

Choose a reason for hiding this comment

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

isn't Object.create(Object.getPrototypeOf({})) same as {}?

Copy link
Contributor

Choose a reason for hiding this comment

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

probably, but with the object literal we ran into type issues again

hash.set(source, clone);
for (i in source) {
for (const i in source) {
if (hasOwnProp(i)) {
clone[i] = deepCopy((source as any)[i], hash, path + `.${i as string}`);
clone[i] = deepCopy((source as any)[i], hash, path + `.${i}`);
}
}
return clone;
Expand Down
23 changes: 16 additions & 7 deletions packages/workflow/test/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,19 @@ describe('jsonParse', () => {

describe('deepCopy', () => {
it('should deep copy an object', () => {
const serializable = {
x: 1,
y: 2,
toJSON: () => 'x:1,y:2',
};
const object = {
deep: {
props: {
list: [{ a: 1 }, { b: 2 }, { c: 3 }],
},
arr: [1, 2, 3],
},
serializable,
arr: [
{
prop: {
Expand All @@ -34,17 +40,18 @@ describe('deepCopy', () => {
},
],
func: () => {},
date: new Date(),
date: new Date(1667389172201),
undef: undefined,
nil: null,
bool: true,
num: 1,
};
const copy = deepCopy(object);
expect(copy).toEqual(object);
expect(copy).not.toBe(object);
expect(copy.arr).toEqual(object.arr);
expect(copy.arr).not.toBe(object.arr);
expect(copy.date).toBe('2022-11-02T11:39:32.201Z');
expect(copy.serializable).toBe(serializable.toJSON());
expect(copy.deep.props).toEqual(object.deep.props);
expect(copy.deep.props).not.toBe(object.deep.props);
});
Expand All @@ -65,7 +72,7 @@ describe('deepCopy', () => {
},
],
func: () => {},
date: new Date(),
date: new Date(1667389172201),
undef: undefined,
nil: null,
bool: true,
Expand All @@ -74,14 +81,16 @@ describe('deepCopy', () => {

object.circular = object;
object.deep.props.circular = object;
object.deep.arr.push(object)
object.deep.arr.push(object);

const copy = deepCopy(object);
expect(copy).toEqual(object);
expect(copy).not.toBe(object);
expect(copy.arr).toEqual(object.arr);
expect(copy.arr).not.toBe(object.arr);
expect(copy.deep.props).toEqual(object.deep.props);
expect(copy.deep.props).not.toBe(object.deep.props);
expect(copy.date).toBe('2022-11-02T11:39:32.201Z');
expect(copy.deep.props.circular).toBe(copy);
expect(copy.deep.props.circular).not.toBe(object);
expect(copy.deep.arr.slice(-1)[0]).toBe(copy);
expect(copy.deep.arr.slice(-1)[0]).not.toBe(object);
});
});