From d78c21d59e46ba44288561bb78b46fdec226d81d Mon Sep 17 00:00:00 2001 From: Oleh Savka Date: Wed, 16 Sep 2026 13:05:08 +0300 Subject: [PATCH] fix(isEqual): track ancestors, not every value the walk has seen A reference repeated in two sibling branches is not a circular reference, but refSet never released anything once the walk left a branch. The second visit found it in the set, warned, and returned false for deeply-equal input: const shared = []; isEqual({ errors: shared, warnings: shared }, { errors: [], warnings: [] }); // false, and "Warning: There may be circular references" A cycle is a value reachable from itself, so the set has to hold the current path rather than the whole history. Entries are now released as the walk unwinds. Genuine cycles are still detected, because a self-referencing value is still its own ancestor when it is reached again. The tests gain a resetWarned() in beforeEach: warning is warningOnce, so a message emitted by an earlier case is suppressed in every later one, and asserting that nothing warned would otherwise prove nothing. Closes #816 --- src/isEqual.ts | 38 +++++++++++++++++----------- src/test/isEqual.test.ts | 54 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 76 insertions(+), 16 deletions(-) diff --git a/src/isEqual.ts b/src/isEqual.ts index a4a29b79..7eb831c8 100644 --- a/src/isEqual.ts +++ b/src/isEqual.ts @@ -9,6 +9,9 @@ import warning from './warning'; */ function isEqual(obj1: any, obj2: any, shallow = false): boolean { // https://github.com/mapbox/mapbox-gl-js/pull/5979/files#diff-fde7145050c47cc3a306856efd5f9c3016e86e859de9afbd02c879be5067e58f + // The ANCESTORS of the value being compared, not every value seen so far: a cycle is a value + // reachable from itself, while the same reference met again in a sibling branch is a repeat and + // has to compare normally. Entries are released as the walk unwinds, below. const refSet = new Set(); function deepEqual(a: any, b: any, level = 1): boolean { const circular = refSet.has(a); @@ -24,26 +27,31 @@ function isEqual(obj1: any, obj2: any, shallow = false): boolean { } refSet.add(a); const newLevel = level + 1; - if (Array.isArray(a)) { - if (!Array.isArray(b) || a.length !== b.length) { - return false; - } - for (let i = 0; i < a.length; i++) { - if (!deepEqual(a[i], b[i], newLevel)) { + try { + if (Array.isArray(a)) { + if (!Array.isArray(b) || a.length !== b.length) { return false; } + for (let i = 0; i < a.length; i++) { + if (!deepEqual(a[i], b[i], newLevel)) { + return false; + } + } + return true; } - return true; - } - if (a && b && typeof a === 'object' && typeof b === 'object') { - const keys = Object.keys(a); - if (keys.length !== Object.keys(b).length) { - return false; + if (a && b && typeof a === 'object' && typeof b === 'object') { + const keys = Object.keys(a); + if (keys.length !== Object.keys(b).length) { + return false; + } + return keys.every(key => deepEqual(a[key], b[key], newLevel)); } - return keys.every(key => deepEqual(a[key], b[key], newLevel)); + // other + return false; + } finally { + // The walk has left this branch, so `a` is no longer an ancestor of what comes next. + refSet.delete(a); } - // other - return false; } return deepEqual(obj1, obj2); diff --git a/src/test/isEqual.test.ts b/src/test/isEqual.test.ts index 6d9c012c..3cdcd3cd 100644 --- a/src/test/isEqual.test.ts +++ b/src/test/isEqual.test.ts @@ -1,5 +1,5 @@ import isEqual from '../isEqual'; -import warning from '../warning'; +import warning, { resetWarned } from '../warning'; describe('isEqual', () => { let errorSpy: jest.SpyInstance; @@ -8,6 +8,12 @@ describe('isEqual', () => { errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); }); + beforeEach(() => { + // `warning` is warningOnce: without this, a message emitted by an earlier test is suppressed + // here, and asserting that nothing warned would prove nothing. + resetWarned(); + }); + afterEach(() => { errorSpy.mockReset(); }); @@ -103,4 +109,50 @@ describe('isEqual', () => { const valueIsEqual = isEqual(obj, obj2); expect(valueIsEqual).toBe(false); }); + + const circularWarnings = () => + errorSpy.mock.calls.filter(([message]) => + String(message).includes('circular references'), + ); + + it('should equal when one side reuses a reference across keys', () => { + const shared: any[] = []; + + const valueIsEqual = isEqual( + { errors: shared, warnings: shared }, + { errors: [], warnings: [] }, + ); + expect(valueIsEqual).toBe(true); + expect(circularWarnings()).toHaveLength(0); + }); + + it('should equal when one side reuses an object across keys', () => { + const point = { x: 1 }; + + const valueIsEqual = isEqual( + { a: point, b: point }, + { a: { x: 1 }, b: { x: 1 } }, + ); + expect(valueIsEqual).toBe(true); + expect(circularWarnings()).toHaveLength(0); + }); + + it('should equal when one side reuses a reference inside an array', () => { + const point = { x: 1 }; + + const valueIsEqual = isEqual([point, point], [{ x: 1 }, { x: 1 }]); + expect(valueIsEqual).toBe(true); + expect(circularWarnings()).toHaveLength(0); + }); + + it('should still detect a cycle reached through an array', () => { + const a: any = { list: [] }; + a.list.push(a); + const b: any = { list: [] }; + b.list.push(b); + + const valueIsEqual = isEqual(a, b); + expect(valueIsEqual).toBe(false); + expect(circularWarnings().length).toBeGreaterThan(0); + }); });