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.

Sign up for GitHub

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

Jump to bottom

fix: deep merge behavior in flat config #17906

Merged
merged 5 commits into from Dec 29, 2023
Merged

fix: deep merge behavior in flat config #17906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8d84b0b
fix: replicate eslintrc merge behavior in flat config
fasttime Dec 26, 2023
5d435f6
do not merge arrays
fasttime Dec 28, 2023
6e2da74
Remove unused code
fasttime Dec 29, 2023
b6732ba
test for undefined properties
fasttime Dec 29, 2023
fac37ba
fix an edge case with non-enumerable properties
fasttime Dec 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 19 additions & 23 deletions lib/config/flat-config-schema.js
View file
Open in desktop
Expand Up @@ -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.
Expand All @@ -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<string, Map<string, Object>>} [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);

Expand All @@ -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.
Expand All @@ -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__"
Copy link
Member

Choose a reason for hiding this comment

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

If we don't want the __proto__, then I'd suggest using Object.create(null) to create result.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the intent here was just to exclude __proto__ when it's an own property, because the inherited property Object.prototype.__proto__ should not be masked or accidentally overwritten (delete only removes own properties). By contrast, Object.create(null) creates an object with null prototype, that lacks all methods of Object.prototype but could still have an own property called __proto__, so it's not really the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, we could treat __proto__ as a regular property with the usual merge behavior as other properties. We'd just need to take care to use Object.defineProperty rather than a regular assignment (=) to set __proto__ in order to not mess up with the prototype (here and here).

Copy link
Member

Choose a reason for hiding this comment

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

Ah gotcha, so this line is deleting an own property that might have been added through the spread operations. That makes sense to me. 👍


// 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;
}
}

Expand Down
171 changes: 138 additions & 33 deletions tests/lib/config/flat-config-schema.js
View file
Open in desktop
Expand Up @@ -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", () => {

Expand All @@ -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);
Comment on lines -21 to -44
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe that these cases are not actually supported in eslintrc or in flat config, because arrays are never merged directly. So I've replaced them with similar unit tests below where arrays are wrapped in a property.

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", () => {
Expand All @@ -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", () => {
Expand All @@ -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", () => {
Expand All @@ -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", () => {
Expand All @@ -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", () => {
Expand All @@ -89,39 +129,104 @@ 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", () => {
const first = { ["__proto__"]: { foo: 42 } };
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", () => {
Expand Down