-
-
Notifications
You must be signed in to change notification settings - Fork 38
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.
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
base: main
Are you sure you want to change the base?
fix: tighten types of MergeStrategy in object-schema
#348
Conversation
| * @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. |
There was a problem hiding this comment.
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.
| * @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. |
There was a problem hiding this comment.
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.
| * 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. |
There was a problem hiding this comment.
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:
- with
value1 = undefinedandvalue2 = { bar: true } - with
value1 = { bar: true }andvalue2 = { baz: false }
| 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>; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves type safety in the @eslint/object-schema package by replacing the overly broad any return types in MergeStrategy methods with more precise generic types. This enhances the developer experience by providing better type inference and compile-time checking when using these merge strategies.
- Uses TypeScript generics to narrow return types from
anyto specific inferred types - Adds comprehensive type tests to verify the new type behavior
- Maintains backward compatibility as the runtime behavior is unchanged
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
packages/object-schema/src/merge-strategy.js |
Updated JSDoc type annotations to use generic templates for overwrite, replace, and assign methods, providing more precise return types |
packages/object-schema/tests/types/types.test.ts |
Added comprehensive type tests to validate that the new generic types correctly infer return types and properly reject invalid type assertions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Prerequisites checklist
What is the purpose of this pull request?
Which packages would you like to change?
@eslint/compat@eslint/config-array@eslint/config-helpers@eslint/core@eslint/mcp@eslint/migrate-config@eslint/object-schema@eslint/plugin-kitWhat problem do you want to solve?
In the public API of
@eslint/object-schema, the return types ofMergeStrategy.overwrite,MergeStrategy.replace, andMergeStrategy.assignare currently typed asany, which feels too broad and could be narrowed to a clearer type.What do you think is the correct solution?
For example:
MergeStrategy.overwritealways returnsvalue2;MergeStrategy.replacealways returns eithervalue1orvalue2; andMergeStrategy.assignalways returns the merged object produced byObject.assign().I think if we use a template for these static functions, we can make the types clearer.
What changes did you make? (Give an overview)
In this PR, I've tightened the types for
MergeStrategyinobject-schema.The previous return type
anywas too broad, and from a TypeScript perspective, usinganyis generally not recommended.By using templates, the return types can be narrowed, so I went ahead with this approach.
Related Issues
N/A
Is there anything you'd like reviewers to focus on?
N/A