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

deepCopy() doesn't work with objects that aren't Array or Object #25

Open
melloc opened this issue Sep 22, 2017 · 2 comments
Open

deepCopy() doesn't work with objects that aren't Array or Object #25

melloc opened this issue Sep 22, 2017 · 2 comments

Comments

@melloc
Copy link
Contributor

melloc commented Sep 22, 2017

If you try to use jsprim.deepCopy() on something where typeof (obj) === "object", and it isn't an Array or Object, then the function will return the original input, instead of creating a copy. More work might be needed, but this was my initial attempt at fixing it:

function deepCopy(obj)
{
	var proto;
	var ret, keys, key;
	var marker = '__deepCopy';

	/*
	 * It must be a primitive type -- just return it.
	 */
	if (typeof (obj) !== 'object')
		return (obj);

	if (obj === null)
		return (null);

	if (obj && obj[marker])
		throw (new Error('attempted deep copy of cyclic object'));

	if (Array.isArray(obj)) {
		ret = [];
		obj[marker] = true;

		for (key = 0; key < obj.length; key++)
			ret.push(deepCopy(obj[key]));

		delete (obj[marker]);
		return (ret);
	}

	if (obj instanceof Date) {
		return (new Date(obj.valueOf()));
	}

	proto = Object.getPrototypeOf(obj);
	ret = {};
	obj[marker] = true;

	keys = Object.getOwnPropertyNames(obj);
	for (var i = 0; i < keys.length; i++) {
		key = keys[i];

		if (key == marker)
			continue;

		ret[key] = Object.getOwnPropertyDescriptor(obj, key);
		if (ret[key].hasOwnProperty('value')) {
			ret[key].value = deepCopy(ret[key].value);
		}
	}

	delete (obj[marker]);

	return (Object.create(proto, ret));
}
@davepacheco
Copy link
Contributor

As we were discussing in the recent review, this was explicitly outside the scope of the original deepCopy. The docs tried to convey this, but not very well. Sorry for the confusion about that.

This was out of scope because I think the desired semantics of copying user-defined types are pretty use-case-specific. Sometimes it's valid to copy an object property-for-property, and sometimes that's totally invalid. (That seems to be why C++ has copy constructors ... and patterns for disabling copy constructors.) There are also lots of edge cases, like getters and setters. Plus, if you're given an object constructed by user function Foo, is it actually a copy if you didn't call Foo on it?

If the goal is to be able to copy objects like Date and RegExp, that sounds much more doable (although it might be challenging if those types evolve in future versions of the language).

@melloc
Copy link
Contributor Author

melloc commented Sep 22, 2017

I think that it would be useful for deepCopy to throw an Error when it isn't going to copy the object. When I'd tried replacing clone with deepCopy in NAPI a while back, I had things start failing in odd ways because they no longer had a real copy, but instead someone else's object. Throwing an error would help make it clear that the copy failed, and prevent you from accidentally using the object. (I would also argue that if the expected use of the function is only to operate on JSON types, then using it for anything else would be a programmer error.)

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

No branches or pull requests

2 participants