Skip to content

fix: tighten types of MergeStrategy in object-schema #348

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

Open
lumirlumir wants to merge 2 commits into
base: main
Choose a base branch
from
+93 −9
Open

fix: tighten types of MergeStrategy in object-schema #348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2075330
fix: tighten types of `MergeStrategy` in `object-schema`
lumirlumir Dec 31, 2025
7bc2052
Merge branch 'main' into fix/tighten-types-of-mergestrategy-in-object…
lumirlumir Jan 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions packages/object-schema/src/merge-strategy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@
export class MergeStrategy {
/**
* Merges two keys by overwriting the first with the second.
* @param {*} value1 The value from the first object key.
* @param {*} value2 The value from the second object key.
* @returns {*} The second value.
* @template TValue1 The type of the value from the first object key.
* @template TValue2 The type of the value from the second object key.
* @param {TValue1} value1 The value from the first object key.
* @param {TValue2} value2 The value from the second object key.
* @returns {TValue2} The second value.
*/
static overwrite(value1, value2) {
return value2;
Expand All @@ -23,9 +25,11 @@ export class MergeStrategy {
/**
* Merges two keys by replacing the first with the second only if the
* second is defined.
* @param {*} value1 The value from the first object key.
* @param {*} value2 The value from the second object key.
* @returns {*} The second value if it is defined.
* @template TValue1 The type of the value from the first object key.
* @template TValue2 The type of the value from the second object key.
* @param {TValue1} value1 The value from the first object key.
* @param {TValue2} value2 The value from the second object key.
* @returns {TValue1 | TValue2} The second value if it is defined.
Copy link
Member Author

@lumirlumir lumirlumir Jan 2, 2026

Choose a reason for hiding this comment

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

Since the replace static method always returns value2 when value2 is not undefined, the return type could be narrowed.

However, there are edge cases where value2 may be, for example, string | undefined, and I don’t see a clear way to express a more specific type for this function right now.

Using TValue1 | TValue2 as the return type seems like the most general approach here. If there’s a way to narrow it more precisely, I’m happy to follow it.

*/
static replace(value1, value2) {
if (typeof value2 !== "undefined") {
Expand All @@ -37,9 +41,11 @@ export class MergeStrategy {

/**
* Merges two properties by assigning properties from the second to the first.
* @param {*} value1 The value from the first object key.
* @param {*} value2 The value from the second object key.
* @returns {*} A new object containing properties from both value1 and
* @template {Record<string | number | symbol, unknown>} TValue1 The type of the value from the first object key.
* @template {Record<string | number | symbol, unknown>} TValue2 The type of the value from the second object key.
Comment on lines +44 to +45
Copy link
Member Author

@lumirlumir lumirlumir Jan 2, 2026

Choose a reason for hiding this comment

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

According to the JSDoc comments and the README.md, it seems the restriction on value1 and value2 is that they must be objects with properties, but I'm not entirely sure.

rewrite/packages/object-schema/src/merge-strategy.js

Line 39 in b65204d

* Merges two properties by assigning properties from the second to the first.

rewrite/packages/object-schema/README.md

Line 97 in b65204d

- `"assign"` - use `Object.assign()` to merge the two values into one object.

Copy link
Member

@fasttime fasttime Jan 15, 2026

Choose a reason for hiding this comment

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

It looks like value1 can be undefined. This is the relevant test case:

rewrite/packages/object-schema/tests/object-schema.test.js

Lines 264 to 274 in 7255100

schema = new ObjectSchema({
foo: {
merge: "assign",
validate() {},
},
});
const result = schema.merge(
{ foo: { bar: true } },
{ foo: { baz: false } },
);

Stepping through with the debugger shows that MergeStrategy.assign() is invoked twice:

  1. with value1 = undefined and value2 = { bar: true }
  2. with value1 = { bar: true } and value2 = { baz: false }

* @param {TValue1} value1 The value from the first object key.
* @param {TValue2} value2 The value from the second object key.
* @returns {Omit<TValue1, keyof TValue2> & TValue2} A new object containing properties from both value1 and
* value2.
*/
static assign(value1, value2) {
Expand Down
78 changes: 78 additions & 0 deletions packages/object-schema/tests/types/types.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,81 @@
//-----------------------------------------------------------------------------

import "@eslint/object-schema";
import { MergeStrategy } from "@eslint/object-schema";

//-----------------------------------------------------------------------------
// Tests
//-----------------------------------------------------------------------------

// #region MergeStrategy

MergeStrategy.overwrite(1, 2) satisfies 2;
MergeStrategy.overwrite("a", "b") satisfies "b";
MergeStrategy.overwrite(true, false) satisfies false;
MergeStrategy.overwrite({ a: 1 }, { b: 2 }) satisfies { b: 2 };

// @ts-expect-error Type 'number' does not satisfy the expected type 'string'.
MergeStrategy.overwrite(1, 2) satisfies string;
// @ts-expect-error Type 'number' does not satisfy the expected type 'boolean'.
MergeStrategy.overwrite(1, 2) satisfies boolean;
// @ts-expect-error Type 'string' does not satisfy the expected type 'number'.
MergeStrategy.overwrite("a", "b") satisfies number;
// @ts-expect-error Type 'string' does not satisfy the expected type 'boolean'.
MergeStrategy.overwrite("a", "b") satisfies boolean;
// @ts-expect-error Type 'boolean' does not satisfy the expected type 'number'.
MergeStrategy.overwrite(true, false) satisfies number;
// @ts-expect-error Type 'boolean' does not satisfy the expected type 'string'.
MergeStrategy.overwrite(true, false) satisfies string;

MergeStrategy.replace(1, 2) satisfies number;
MergeStrategy.replace("a", "b") satisfies string;
MergeStrategy.replace(true, false) satisfies boolean;
MergeStrategy.replace({ a: 1 }, { b: 2 }) satisfies Record<string, number>;
Comment on lines +37 to +40
Copy link
Member Author

@lumirlumir lumirlumir Jan 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I intentionally wrote this type test a little loosely, per the #348 (comment), in case the types can be narrowed further in the future.

For example, we can use the following test case instead to ensure its behavior:

- MergeStrategy.replace(1, 2) satisfies number
+ MergeStrategy.replace(1, 2) satisfies 1 | 2

(FYI: this type test is also ensured and fails if we revert this change.)

However, If narrowing the type test is needed here as well, I'm happy to do that.


// @ts-expect-error Type 'number' does not satisfy the expected type 'string'.
MergeStrategy.replace(1, 2) satisfies string;
// @ts-expect-error Type 'number' does not satisfy the expected type 'boolean'.
MergeStrategy.replace(1, 2) satisfies boolean;
// @ts-expect-error Type 'string' does not satisfy the expected type 'number'.
MergeStrategy.replace("a", "b") satisfies number;
// @ts-expect-error Type 'string' does not satisfy the expected type 'boolean'.
MergeStrategy.replace("a", "b") satisfies boolean;
// @ts-expect-error Type 'boolean' does not satisfy the expected type 'number'.
MergeStrategy.replace(true, false) satisfies number;
// @ts-expect-error Type 'boolean' does not satisfy the expected type 'string'.
MergeStrategy.replace(true, false) satisfies string;

const sym1: unique symbol = Symbol("sym1");
const sym2: unique symbol = Symbol("sym2");
MergeStrategy.assign({ [sym1]: 1 }, { [sym2]: true }) satisfies {
[sym1]: number;
[sym2]: boolean;
};
MergeStrategy.assign({ 1: 1 }, { 2: 2 }) satisfies {
1: number;
2: number;
};
MergeStrategy.assign({ a: 1 }, { b: 2 }) satisfies {
a: number;
b: number;
};
MergeStrategy.assign({ a: 1 } as const, { b: 2 } as const) satisfies {
readonly a: 1;
readonly b: 2;
};
MergeStrategy.assign({ a: 1 }, { a: "a" }) satisfies {
a: "a";
};

// @ts-expect-error Type 'number' is not assignable to parameter of type 'Record<string | number | symbol, unknown>'.
MergeStrategy.assign(1, 2);
// @ts-expect-error Type 'string' is not assignable to parameter of type 'Record<string | number | symbol, unknown>'.
MergeStrategy.assign("a", "b");
// @ts-expect-error Type 'boolean' is not assignable to parameter of type 'Record<string | number | symbol, unknown>'.
MergeStrategy.assign(true, false);
// @ts-expect-error `{ a: "a" }` should overwrite `{ a: 1 }`.
MergeStrategy.assign({ a: 1 }, { a: "a" }) satisfies {
a: 1;
};

// #endregion MergeStrategy