From 9852a31edcf054bd5d15753ef18e2ad3216b1b71 Mon Sep 17 00:00:00 2001 From: Nitin Kumar Date: Thu, 1 Feb 2024 17:54:36 +0530 Subject: [PATCH] fix: deep merge behavior in flat config (#18065) * fix: replicate eslintrc merge behavior in flat config * do not merge arrays * Remove unused code * test for undefined properties * fix an edge case with non-enumerable properties (cherry picked from commit f182114144ae0bb7187de34a1661f31fb70f1357) Co-authored-by: Francesco Trotta --- lib/config/flat-config-schema.js | 42 +++--- tests/lib/config/flat-config-schema.js | 171 ++++++++++++++++++++----- 2 files changed, 157 insertions(+), 56 deletions(-) diff --git a/lib/config/flat-config-schema.js b/lib/config/flat-config-schema.js index 55546134ec3..6b64319c8fc 100644 --- a/lib/config/flat-config-schema.js +++ b/lib/config/flat-config-schema.js @@ -53,6 +53,15 @@ function isNonNullObject(value) { return typeof value === "object" && value !== null; } +/** + * Check if a value is a non-null non-array object. + * @param {any} value The value to check. + * @returns {boolean} `true` if the value is a non-null non-array object. + */ +function isNonArrayObject(value) { + return isNonNullObject(value) && !Array.isArray(value); +} + /** * Check if a value is undefined. * @param {any} value The value to check. @@ -62,25 +71,14 @@ function isUndefined(value) { return typeof value === "undefined"; } -// A unique empty object to be used internally as a mapping key in `deepMerge`. -const EMPTY_OBJECT = {}; - /** - * Deeply merges two objects. + * Deeply merges two non-array objects. * @param {Object} first The base object. - * @param {any} second The overrides value. + * @param {Object} second The overrides object. * @param {Map>} [mergeMap] Maps the combination of first and second arguments to a merged result. * @returns {Object} An object with properties from both first and second. */ -function deepMerge(first, second = {}, mergeMap = new Map()) { - - /* - * If the second value is an array, just return it. We don't merge - * arrays because order matters and we can't know the correct order. - */ - if (Array.isArray(second)) { - return second; - } +function deepMerge(first, second, mergeMap = new Map()) { let secondMergeMap = mergeMap.get(first); @@ -98,7 +96,7 @@ function deepMerge(first, second = {}, mergeMap = new Map()) { } /* - * First create a result object where properties from the second value + * First create a result object where properties from the second object * overwrite properties from the first. This sets up a baseline to use * later rather than needing to inspect and change every property * individually. @@ -108,27 +106,25 @@ function deepMerge(first, second = {}, mergeMap = new Map()) { ...second }; + delete result.__proto__; // eslint-disable-line no-proto -- don't merge own property "__proto__" + // Store the pending result for this combination of first and second arguments. secondMergeMap.set(second, result); for (const key of Object.keys(second)) { // avoid hairy edge case - if (key === "__proto__") { + if (key === "__proto__" || !Object.prototype.propertyIsEnumerable.call(first, key)) { continue; } const firstValue = first[key]; const secondValue = second[key]; - if (isNonNullObject(firstValue)) { + if (isNonArrayObject(firstValue) && isNonArrayObject(secondValue)) { result[key] = deepMerge(firstValue, secondValue, mergeMap); - } else if (isUndefined(firstValue)) { - if (isNonNullObject(secondValue)) { - result[key] = deepMerge(EMPTY_OBJECT, secondValue, mergeMap); - } else if (!isUndefined(secondValue)) { - result[key] = secondValue; - } + } else if (isUndefined(secondValue)) { + result[key] = firstValue; } } diff --git a/tests/lib/config/flat-config-schema.js b/tests/lib/config/flat-config-schema.js index 1cde41ab16b..b1b7303af62 100644 --- a/tests/lib/config/flat-config-schema.js +++ b/tests/lib/config/flat-config-schema.js @@ -7,6 +7,28 @@ const { flatConfigSchema } = require("../../../lib/config/flat-config-schema"); const { assert } = require("chai"); +const { Legacy: { ConfigArray } } = require("@eslint/eslintrc"); + +/** + * This function checks the result of merging two values in eslintrc config. + * It uses deep strict equality to compare the actual and the expected results. + * This is useful to ensure that the flat config merge logic behaves similarly to the old logic. + * When eslintrc is removed, this function and its invocations can be also removed. + * @param {Object} [first] The base object. + * @param {Object} [second] The overrides object. + * @param {Object} [expectedResult] The expected reults of merging first and second values. + * @returns {void} + */ +function confirmLegacyMergeResult(first, second, expectedResult) { + const configArray = new ConfigArray( + { settings: first }, + { settings: second } + ); + const config = configArray.extractConfig("/file"); + const actualResult = config.settings; + + assert.deepStrictEqual(actualResult, expectedResult); +} describe("merge", () => { @@ -18,36 +40,14 @@ describe("merge", () => { const result = merge(first, second); assert.deepStrictEqual(result, { ...first, ...second }); - }); - - it("overrides an object with an array", () => { - const first = { foo: 42 }; - const second = ["bar", "baz"]; - const result = merge(first, second); - - assert.strictEqual(result, second); - }); - - it("merges an array with an object", () => { - const first = ["foo", "bar"]; - const second = { baz: 42 }; - const result = merge(first, second); - - assert.deepStrictEqual(result, { 0: "foo", 1: "bar", baz: 42 }); - }); - - it("overrides an array with another array", () => { - const first = ["foo", "bar"]; - const second = ["baz", "qux"]; - const result = merge(first, second); - - assert.strictEqual(result, second); + confirmLegacyMergeResult(first, second, result); }); it("returns an emtpy object if both values are undefined", () => { const result = merge(void 0, void 0); assert.deepStrictEqual(result, {}); + confirmLegacyMergeResult(void 0, void 0, result); }); it("returns an object equal to the first one if the second one is undefined", () => { @@ -56,6 +56,7 @@ describe("merge", () => { assert.deepStrictEqual(result, first); assert.notStrictEqual(result, first); + confirmLegacyMergeResult(first, void 0, result); }); it("returns an object equal to the second one if the first one is undefined", () => { @@ -64,6 +65,16 @@ describe("merge", () => { assert.deepStrictEqual(result, second); assert.notStrictEqual(result, second); + confirmLegacyMergeResult(void 0, second, result); + }); + + it("does not preserve the type of merged objects", () => { + const first = new Set(["foo", "bar"]); + const second = new Set(["baz"]); + const result = merge(first, second); + + assert.deepStrictEqual(result, {}); + confirmLegacyMergeResult(first, second, result); }); it("merges two objects in a property", () => { @@ -72,6 +83,34 @@ describe("merge", () => { const result = merge(first, second); assert.deepStrictEqual(result, { foo: { bar: "baz", qux: 42 } }); + confirmLegacyMergeResult(first, second, result); + }); + + it("overwrites an object in a property with an array", () => { + const first = { someProperty: { 1: "foo", bar: "baz" } }; + const second = { someProperty: ["qux"] }; + const result = merge(first, second); + + assert.deepStrictEqual(result, second); + assert.strictEqual(result.someProperty, second.someProperty); + }); + + it("overwrites an array in a property with another array", () => { + const first = { someProperty: ["foo", "bar", void 0, "baz"] }; + const second = { someProperty: ["qux", void 0, 42] }; + const result = merge(first, second); + + assert.deepStrictEqual(result, second); + assert.strictEqual(result.someProperty, second.someProperty); + }); + + it("overwrites an array in a property with an object", () => { + const first = { foo: ["foobar"] }; + const second = { foo: { 1: "qux", bar: "baz" } }; + const result = merge(first, second); + + assert.deepStrictEqual(result, second); + assert.strictEqual(result.foo, second.foo); }); it("does not override a value in a property with undefined", () => { @@ -81,6 +120,7 @@ describe("merge", () => { assert.deepStrictEqual(result, first); assert.notStrictEqual(result, first); + confirmLegacyMergeResult(first, second, result); }); it("does not change the prototype of a merged object", () => { @@ -89,6 +129,7 @@ describe("merge", () => { const result = merge(first, second); assert.strictEqual(Object.getPrototypeOf(result), Object.prototype); + confirmLegacyMergeResult(first, second, result); }); it("does not merge the '__proto__' property", () => { @@ -96,32 +137,96 @@ describe("merge", () => { const second = { ["__proto__"]: { bar: "baz" } }; const result = merge(first, second); - assert.deepStrictEqual(result, second); - assert.notStrictEqual(result, second); + assert.deepStrictEqual(result, {}); + confirmLegacyMergeResult(first, second, result); }); - it("throws an error if a value in a property is overriden with null", () => { + it("overrides a value in a property with null", () => { const first = { foo: { bar: "baz" } }; const second = { foo: null }; + const result = merge(first, second); - assert.throws(() => merge(first, second), TypeError); + assert.deepStrictEqual(result, second); + assert.notStrictEqual(result, second); + confirmLegacyMergeResult(first, second, result); }); - it("does not override a value in a property with a primitive", () => { + it("overrides a value in a property with a non-nullish primitive", () => { const first = { foo: { bar: "baz" } }; const second = { foo: 42 }; const result = merge(first, second); - assert.deepStrictEqual(result, first); - assert.notStrictEqual(result, first); + assert.deepStrictEqual(result, second); + assert.notStrictEqual(result, second); + confirmLegacyMergeResult(first, second, result); }); - it("merges an object in a property with a string", () => { + it("overrides an object in a property with a string", () => { const first = { foo: { bar: "baz" } }; const second = { foo: "qux" }; const result = merge(first, second); - assert.deepStrictEqual(result, { foo: { 0: "q", 1: "u", 2: "x", bar: "baz" } }); + assert.deepStrictEqual(result, second); + assert.notStrictEqual(result, first); + confirmLegacyMergeResult(first, second, result); + }); + + it("overrides a value in a property with a function", () => { + const first = { someProperty: { foo: 42 } }; + const second = { someProperty() {} }; + const result = merge(first, second); + + assert.deepStrictEqual(result, second); + assert.notProperty(result.someProperty, "foo"); + confirmLegacyMergeResult(first, second, result); + }); + + it("overrides a function in a property with an object", () => { + const first = { someProperty: Object.assign(() => {}, { foo: "bar" }) }; + const second = { someProperty: { baz: "qux" } }; + const result = merge(first, second); + + assert.deepStrictEqual(result, second); + assert.notProperty(result.someProperty, "foo"); + confirmLegacyMergeResult(first, second, result); + }); + + it("sets properties to undefined", () => { + const first = { foo: void 0, bar: void 0 }; + const second = { foo: void 0, baz: void 0 }; + const result = merge(first, second); + + assert.deepStrictEqual(result, { foo: void 0, bar: void 0, baz: void 0 }); + }); + + it("considers only own enumerable properties", () => { + const first = { + __proto__: { inherited1: "A" }, // non-own properties are not considered + included1: "B", + notMerged1: { first: true } + }; + const second = { + __proto__: { inherited2: "C" }, // non-own properties are not considered + included2: "D", + notMerged2: { second: true } + }; + + // non-enumerable properties are not considered + Object.defineProperty(first, "notMerged2", { enumerable: false, value: { first: true } }); + Object.defineProperty(second, "notMerged1", { enumerable: false, value: { second: true } }); + + const result = merge(first, second); + + assert.deepStrictEqual( + result, + { + included1: "B", + included2: "D", + notMerged1: { first: true }, + notMerged2: { second: true } + } + ); + confirmLegacyMergeResult(first, second, result); }); it("merges objects with self-references", () => {