feat(utils): resolve common function ts error #2903
Conversation
WalkthroughThe pull request updates several utility files by enhancing type safety and flexibility. In the tree model and validation modules, optional parameters have been introduced to method signatures and function parameters modified to accept objects. Type annotations have been added or refined across upload, schema, and utility functions. Additionally, minor logic adjustments (such as converting numbers to strings during formatting) have been made without altering core functionality. Finally, a new TypeScript compiler option ( Changes
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/utils/src/tree-model/node.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/utils/src/upload-ajax/index.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/utils/src/validate/schema.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
WalkthroughThis pull request addresses TypeScript errors in common utility functions by adding type annotations and interfaces. It enhances type safety and resolves existing issues without introducing breaking changes. Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| const deep = isDeep(rule, data) | ||
|
|
||
| failds = arrayed(failds) | ||
|
|
||
| if (!options.suppressWarning && failds.length) { | ||
| Schema.warning('async-validator:', failds) | ||
| Schema.warning() |
There was a problem hiding this comment.
The removal of the async-validator: prefix in the Schema.warning call may lead to loss of context in log messages. Consider maintaining the prefix or ensuring that the logging mechanism provides sufficient context.
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/utils/src/validate/rules/required.ts (1)
16-23: LGTM! Well-structured interface definition.The
RequiredTypeinterface clearly defines the expected parameter types and correctly marks optional parameters.Consider adding JSDoc comments to document the purpose of each interface property.
packages/utils/src/upload-ajax/index.ts (2)
41-41: Consider using a more specific type instead ofany.The
errorvariable could use a more specific type likeError & { status?: number; method?: string; url?: string }to better represent its structure.- const error: any = new Error(errorText) + const error: Error & { status?: number; method?: string; url?: string } = new Error(errorText)
59-59: Use ProgressEvent type instead ofany.The event parameter should use the built-in
ProgressEventtype for better type safety.- xhr.upload.onprogress = (event: any) => { + xhr.upload.onprogress = (event: ProgressEvent) => {packages/utils/src/validate/util.ts (2)
112-112: Consider documenting the default behavior.Making
dataTypeoptional is good, but please document the default behavior when it's not provided.Add JSDoc comment explaining the default behavior:
+/** + * @param data The value to check for emptiness + * @param dataType Optional type of the data. If not provided, only null check is performed + */ export function isEmptyValue(data, dataType?) {
138-138: Use more specific types instead ofany[].Consider using more specific types for the arrays based on their content.
- const results = [] as any[] + interface ValidationError { + field: string; + message: string; + } + const results = [] as ValidationError[]Also applies to: 186-186, 224-224
packages/utils/src/validate/schema.ts (1)
16-16: Consider using proper TypeScript interfaces instead of optional parameters.Instead of making
translateoptional, consider creating an interface for the Schema constructor parameters to better document the expected types.+interface SchemaOptions { + descriptor: any; + translate?: any; +} -function Schema(descriptor, translate?) { +function Schema(options: SchemaOptions) { + const { descriptor, translate } = options;packages/utils/tsconfig.json (1)
15-15:❓ Verification inconclusive
Consider keeping
noImplicitAnyenabled for better type safety.Disabling
noImplicitAnyreduces TypeScript's ability to catch potential type-related bugs. While this might help with initial TypeScript adoption, consider:
- Keeping it enabled and properly typing variables
- Using explicit
anytype annotations where absolutely necessary- Creating a migration plan to gradually remove implicit
anyusage
🌐 Web query:
What are the benefits of enabling noImplicitAny in TypeScript?💡 Result:
Enabling the
noImplicitAnycompiler option in TypeScript significantly improves code quality and safety by enforcing stricter type checks. Here are its key benefits:1. Prevents accidental
anytype inference
- Catches cases where TypeScript would silently default variables/parameters to
anywhen types can't be inferred[2][4]- Forces explicit type annotations for untyped function parameters and variables[1][3]
Example of prevented error:
// Without noImplicitAny: Compiles but risks runtime errors function splitString(input) { // input implicitly 'any' return input.split('.'); } // With noImplicitAny: Triggers compile-time error function splitString(input) { // 🚨 Error: Parameter 'input' implicitly has 'any' type return input.split('.'); }2. Enhances type safety
- Reduces runtime errors by catching type mismatches during compilation[3][6]
- Helps identify areas needing proper type declarations during refactoring[7]
- Prevents dangerous operations on
any-typed values[3][6]3. Improves code maintainability
- Creates self-documenting code through explicit type annotations[1][3]
- Makes type-related issues visible during development rather than at runtime[2][4]
- Facilitates safer code evolution by surfacing breaking API changes[7]
4. Encourages better type practices
- Promotes use of proper types instead of
anyescape hatches[6][9]- Guides developers toward TypeScript's full type system capabilities[3][6]
- Works well with other strict mode flags for comprehensive type checking[8]
Comparison table of enabled vs disabled:
Scenario noImplicitAny: falsenoImplicitAny: trueUntyped function param Silently becomes anyCompile-time error Missing return type Inferred as anyError if can't be inferred Third-party JS imports Implicit anyallowedRequires type declarations[1] While enabling
noImplicitAnymay initially require adding type annotations[1][9], it leads to more robust and maintainable TypeScript code in the long term by eliminating hidden type assumptions and preventing entire classes of runtime errors[2][4].Citations:
- 1: https://www.tutorialspoint.com/explain-about-noimplicitany-in-typescript
- 2: https://www.tektutorialshub.com/typescript/noimplicitany-in-typescript/
- 3: https://www.lucaspaganini.com/academy/20-typescript-compiler-options-for-your-tsconfig-json
- 4: https://www.typescriptlang.org/tsconfig/noImplicitAny.html
- 5: https://biomejs.dev/linter/rules/no-implicit-any-let/
- 6: https://dev.to/geraldhamiltonwicks/5-essential-flags-to-enable-on-your-typescript-code-271k
- 7: https://lucid.co/techblog/2018/06/20/how-to-actually-improve-type-safety-with-the-typescript-strict-flags
- 8: https://www.typescriptlang.org/tsconfig/
- 9: https://codepruner.com/how-to-handle-notimplictany-during-migration-from-js-to-ts/
- 10: https://v17.angular.io/guide/typescript-configuration
Action: Re-assess the decision to disable
noImplicitAny(see packages/utils/tsconfig.json, line 15)Disabling
noImplicitAnycan undermine TypeScript’s strict type-checking, allowing implicitanytypes that could lead to runtime bugs and reduced code clarity. EnablingnoImplicitAnyoffers significant benefits by enforcing explicit type annotations and catching potential type mismatches early. If a gradual adoption is required, please consider the following:
- Re-enable
noImplicitAnyto leverage TypeScript’s compiler benefits and improve overall code safety.- Adopt explicit
anyonly where absolutely necessary to minimize unintended type omissions.- Develop a clear migration plan to phase out implicit
anyusage over time while maintaining robust type-checking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/utils/src/tree-model/node.ts(1 hunks)packages/utils/src/upload-ajax/index.ts(2 hunks)packages/utils/src/validate/rules/required.ts(1 hunks)packages/utils/src/validate/rules/type.ts(1 hunks)packages/utils/src/validate/schema.ts(7 hunks)packages/utils/src/validate/util.ts(6 hunks)packages/utils/src/validate/validations/pattern.ts(1 hunks)packages/utils/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/utils/src/validate/validations/pattern.ts
🔇 Additional comments (4)
packages/utils/src/validate/rules/type.ts (1)
88-89: LGTM! Good refactoring to use object parameters.The change to use object parameters improves code maintainability and aligns with the new
RequiredTypeinterface.packages/utils/src/validate/util.ts (2)
70-70: LGTM! Explicit toString() conversion is safer.Using explicit
toString()conversion is safer than implicit type coercion.
200-201: LGTM! Good addition of Promise type annotations.The explicit
Promise<void>type annotations improve code clarity and type safety.Also applies to: 225-226
packages/utils/src/tree-model/node.ts (1)
281-281: LGTM! Optional parameters improve API flexibility.Making
indexandbatchparameters optional is a good improvement as it allows for more flexible usage of theinsertChildmethod while maintaining backward compatibility.
| let errors = [] as any | ||
| let fields = {} as any |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any type assertions with proper types.
Using any type assertions reduces type safety. Consider defining proper interfaces for these variables.
-let errors = [] as any
-let fields = {} as any
+interface ValidationError {
+ field: string;
+ message: string;
+}
+interface ValidationFields {
+ [key: string]: ValidationError[];
+}
+let errors: ValidationError[] = []
+let fields: ValidationFields = {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let errors = [] as any | |
| let fields = {} as any | |
| interface ValidationError { | |
| field: string; | |
| message: string; | |
| } | |
| interface ValidationFields { | |
| [key: string]: ValidationError[]; | |
| } | |
| let errors: ValidationError[] = [] | |
| let fields: ValidationFields = {} |
| Schema.validators = {} as any | ||
|
|
||
| Schema.warning = warning | ||
|
|
||
| Schema.messages = {} | ||
| Schema.messages = {} as any | ||
|
|
||
| Schema.systemMessages = {} | ||
| Schema.systemMessages = {} as any |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define proper types for Schema static properties.
Using any type assertions for static properties reduces type safety. Consider defining proper interfaces.
+interface Validators {
+ [key: string]: (rule: any, value: any, callback: Function, source: any, options: any) => void;
+}
+interface Messages {
+ [key: string]: string;
+}
-Schema.validators = {} as any
-Schema.messages = {} as any
-Schema.systemMessages = {} as any
+Schema.validators: Validators = {}
+Schema.messages: Messages = {}
+Schema.systemMessages: Messages = {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Schema.validators = {} as any | |
| Schema.warning = warning | |
| Schema.messages = {} | |
| Schema.messages = {} as any | |
| Schema.systemMessages = {} | |
| Schema.systemMessages = {} as any | |
| interface Validators { | |
| [key: string]: (rule: any, value: any, callback: Function, source: any, options: any) => void; | |
| } | |
| interface Messages { | |
| [key: string]: string; | |
| } | |
| Schema.validators: Validators = {} | |
| Schema.warning = warning | |
| Schema.messages: Messages = {} | |
| Schema.systemMessages: Messages = {} |
| @@ -85,7 +85,8 @@ const types = { | |||
|
|
|||
| export default function (rule, value, source, errors, options) { | |||
| if (rule.required && value === undefined) { | |||
| required(rule, value, source, errors, options) | |||
| let checkValue = value | |||
| required({ rule, checkValue, source, errors, options }) | |||
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit