Fix handling of circular references in objects (#104)

Co-authored-by: Sindre Sorhus <[email protected]>

index.js

@@ -1,7 +1,8 @@
 /* eslint-disable unicorn/prefer-spread */
-import {TARGET, UNSUBSCRIBE} from './lib/constants.js';
+import {TARGET, UNSUBSCRIBE, PATH_SEPARATOR} from './lib/constants.js';
 import {isBuiltinWithMutableMethods, isBuiltinWithoutMutableMethods} from './lib/is-builtin.js';
 import path from './lib/path.js';
+import isArray from './lib/is-array.js';
 import isSymbol from './lib/is-symbol.js';
 import isIterator from './lib/is-iterator.js';
 import wrapIterator from './lib/wrap-iterator.js';
@@ -74,7 +75,52 @@ const onChange = (object, onChange, options = {}) => {
 			basePath = cache.getPath(target);
 		}
 
-		return cache.getProxy(value, path.concat(basePath, property), handler, proxyTarget);
+		/*
+  		Check for circular references.
+
+  		If the value already has a corresponding path/proxy,
+		and if the path corresponds to one of the parents,
+		then we are on a circular case, where the child is pointing to their parent.
+		In this case we return the proxy object with the shortest path.
+  		*/
+		const childPath = path.concat(basePath, property);
+		const existingPath = cache.getPath(value);
+
+		if (existingPath && isSameObjectTree(childPath, existingPath)) {
+			// We are on the same object tree but deeper, so we use the parent path.
+			return cache.getProxy(value, existingPath, handler, proxyTarget);
+		}
+
+		return cache.getProxy(value, childPath, handler, proxyTarget);
+	};
+
+	/*
+	Returns true if `childPath` is a subpath of `existingPath`
+	(if childPath starts with existingPath). Otherwise, it returns false.
+
+ 	It also returns false if the 2 paths are identical.
+
+ 	For example:
+	- childPath    = group.layers.0.parent.layers.0.value
+	- existingPath = group.layers.0.parent
+	*/
+	const isSameObjectTree = (childPath, existingPath) => {
+		if (isSymbol(childPath) || childPath.length <= existingPath.length) {
+			return false;
+		}
+
+		if (isArray(existingPath) && existingPath.length === 0) {
+			return false;
+		}
+
+		const childParts = isArray(childPath) ? childPath : childPath.split(PATH_SEPARATOR);
+		const existingParts = isArray(existingPath) ? existingPath : existingPath.split(PATH_SEPARATOR);
+
+		if (childParts.length <= existingParts.length) {
+			return false;
+		}
+
+		return !(existingParts.some((part, index) => part !== childParts[index]));
 	};
 
 	const handler = {

tests/on-change.test.js

@@ -751,3 +751,80 @@ test('should not wrap a proxied object in another proxy', t => {
 
 	t.is(proxy.b, proxy.c);
 });
+
+test('path should be the shorter one in the same object for circular references', t => {
+	const layer1 = {value: 0};
+	const layer2 = {value: 0};
+	const layer3 = {value: 0};
+
+	const group = {
+		layers: [layer1, layer2, layer3],
+		value: 0,
+	};
+
+	layer1.group = group;
+	layer2.group = group;
+	layer3.group = group;
+
+	let resultPath;
+
+	const proxy = onChange(group, path => {
+		resultPath = path;
+	});
+
+	proxy.layers[0].value = 11;
+	t.is(resultPath, 'layers.0.value');
+
+	proxy.layers[0].group.value = 22;
+	t.is(resultPath, 'layers.0.group.value');
+
+	proxy.layers[0].group.layers[0].value = 33;
+	t.is(resultPath, 'layers.0.value');
+
+	proxy.layers[1].group.layers[0].group.layers[1].group.layers[1].group.layers[2].value = 33;
+	t.is(resultPath, 'layers.2.value');
+});
+
+test('array path should be the shorter one in the same object for circular references', t => {
+	const layer1 = {value: 0};
+	const layer2 = {value: 0};
+	const layer3 = {value: 0};
+
+	const group = {
+		layers: [layer1, layer2, layer3],
+		value: 0,
+	};
+
+	layer1.group = group;
+	layer2.group = group;
+	layer3.group = group;
+
+	let resultPath;
+
+	const proxy = onChange(group, path => {
+		resultPath = path;
+	}, {
+		pathAsArray: true,
+	});
+
+	proxy.layers[0].value = 11;
+	t.is(resultPath[0], 'layers');
+	t.is(resultPath[1], '0');
+	t.is(resultPath[2], 'value');
+
+	proxy.layers[0].group.value = 22;
+	t.is(resultPath[0], 'layers');
+	t.is(resultPath[1], '0');
+	t.is(resultPath[2], 'group');
+	t.is(resultPath[3], 'value');
+
+	proxy.layers[0].group.layers[0].value = 33;
+	t.is(resultPath[0], 'layers');
+	t.is(resultPath[1], '0');
+	t.is(resultPath[2], 'value');
+
+	proxy.layers[1].group.layers[0].group.layers[1].group.layers[1].group.layers[2].value = 33;
+	t.is(resultPath[0], 'layers');
+	t.is(resultPath[1], '2');
+	t.is(resultPath[2], 'value');
+});