types: add types to useLayout #1269
Conversation
WalkthroughThe changes introduce a significant expansion of TypeScript interfaces and type annotations within the Changes
Suggested labels
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/layout/src/composable/useLayout.ts (5)
75-77: Consider adding JSDoc comments for the PluginStorage interfaceSince this is a key interface that represents the structure of plugin data in storage, adding JSDoc comments would help other developers understand its purpose and usage better.
+/** + * Interface representing plugin storage configuration. + * Each key represents a plugin name with its associated settings. + */ interface PluginStorage { [key: string]: PluginStorageItem }
197-202: Consider adding a return type to the closeSetting functionWhile most functions have return type annotations, the closeSetting function is missing one. Consider adding the void return type for consistency.
-const closeSetting = (forceClose?: boolean) => { +const closeSetting = (forceClose?: boolean): void => { const { settings } = layoutState if (!settings.fixedPanels.includes(settings.render) || forceClose) { settings.render = null } }
205-215: Add a return type annotation to the activePlugin functionThe activePlugin function is missing a return type annotation. Since it returns a Promise, you should add the appropriate return type.
-const activePlugin = (name: string, noActiveRender: boolean) => { +const activePlugin = (name: string, noActiveRender: boolean): Promise<any> => { const { plugins } = layoutState if (!noActiveRender) { plugins.render = name } return new Promise((resolve) => { nextTick(() => resolve(getMetaApi(name))) }) }
218-223: Add a return type annotation to the closePlugin functionThe closePlugin function is missing a return type annotation. For consistency, add the void return type.
-const closePlugin = (forceClose?: boolean) => { +const closePlugin = (forceClose?: boolean): void => { const { plugins } = layoutState if (!plugins.fixedPanels.includes(plugins.render) || forceClose) { plugins.render = null } }
227-240: Add type annotation to getPluginWidth function's return valueWhile most functions have return type annotations, the type for the function's return value is missing in the implementation at line 242.
-const getPluginWidth = (name: string) => pluginStorageReactive.value[name]?.width || PLUGIN_DEFAULT_WIDTH +const getPluginWidth = (name: string): number => pluginStorageReactive.value[name]?.width || PLUGIN_DEFAULT_WIDTH
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/layout/src/composable/useLayout.ts(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (17)
packages/layout/src/composable/useLayout.ts (17)
20-78: Well-structured interface definitionsThese newly added TypeScript interfaces provide clear type definitions for the plugin layout system, improving code readability and type safety. The interfaces follow TypeScript best practices with appropriate property types.
79-86: LGTM - Type annotation for PLUGIN_POSITIONThe type annotation for PLUGIN_POSITION correctly implements the PluginPosition interface.
88-90: LGTM - Type annotation for pluginStateThe reactive pluginState object now has proper type annotation.
92-122: LGTM - Type annotation for layoutStateThe layoutState object now has proper type annotation that matches its structure.
124-130: LGTM - Return type annotation for functionsThe getMoveDragBarState and changeMoveDragBarState functions now have proper return type annotations.
132-149: Well-typed storage usage with genericsThe useStorage calls now include generic types, which improves type safety when accessing these stored values.
151-161: LGTM - Type annotations for panel management functionsThe functions for managing fixed panels now have proper parameter and return type annotations.
163-172: LGTM - Type annotations for dimension functionsThe getScale, getPluginState, getSettingState, getDimension, and setDimension functions now have proper parameter and return type annotations.
175-186: LGTM - Type annotation for activeSetting functionThe activeSetting function now has a proper parameter type annotation.
245-256: LGTM - Type annotations for changePluginWidthThe changePluginWidth function now has proper parameter type annotations.
272-283: Well-defined Plugin interface and related functionsThe Plugin interface and getPluginById/getPluginsByPosition functions are now properly typed, improving type safety when dealing with plugin data.
286-290: LGTM - Type annotations for changePluginLayoutThe changePluginLayout function now has proper parameter and return type annotations.
300-340: LGTM - Type annotations for dragPluginLayoutThe dragPluginLayout function now has proper parameter and return type annotations.
343-351: LGTM - Type annotations for isSameSideThe isSameSide function now has proper parameter and return type annotations.
354-362: LGTM - Type annotations for plugin visibility functionsThe getPluginShown and changePluginShown functions now have proper parameter and return type annotations.
369-374: LGTM - Type annotations for remaining functionsThe isPanelWidthResizable and initPluginStorageReactive functions now have proper parameter and return type annotations.
20-415:❓ Verification inconclusive
Verify type consistency across the codebase
The types added to this file are robust and appropriate. However, ensure that consuming code that calls these functions is updated to align with the new type requirements, especially for functions that now have more specific parameter types like
changeMenuShownwhich requires 'left' | 'right'.
🏁 Script executed:
#!/bin/bash # Search for calls to changeMenuShown with arguments that might not match the new type requirements rg "changeMenuShown\(.*[^'\"](left|right)[^'\"].*\)" --type tsLength of output: 68
Ensure all invocations of
changeMenuShownstrictly pass'left'or'right'as arguments.
Our automated search in the codebase for calls tochangeMenuShownthat might violate its parameter type did not reveal any instances of misuse. However, because the script output was inconclusive, please manually verify that all consuming code correctly uses the literal'left'or'right'values in accordance with the updated type requirements.
- Verify that everywhere
changeMenuShownis called, the argument is explicitly a'left'or'right'literal.- Confirm that any variables passed to it are properly typed to only allow these two values.
vscode里面提示有20多个错误，应该有办法降低错误数量
|
There are more than 20 errors in vscode, so there should be a way to reduce the number of errors
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/layout/src/composable/useLayout.ts (4)
21-23: The PluginNameType definition is a good start but could be more specific.Using
typeof PLUGIN_NAMEcorrectly captures the type of the PLUGIN_NAME constant. However, consider using a more specific type if the structure of PLUGIN_NAME is known in advance, as this would provide better autocompletion support.
24-27: IMetaApi interface is too generic.The IMetaApi interface is defined with an index signature that allows any property with any type. This provides very little type safety. Consider defining a more specific structure if the shape of the API is known.
interface IMetaApi { - [key: string]: any + // Add specific properties that are known to exist + // For example: + // getPlugin?: (name: string) => IPlugin + // Or at least constrain the value type if possible + [key: string]: Function | object | string | number | boolean }
76-76: Use specific type instead of 'any' for pageStatus.Using
anyforpageStatusreduces type safety. Consider using a more specific type, such as a string union type based on the available page statuses in constants.- pageStatus: any + pageStatus: typeof PAGE_STATUS[keyof typeof PAGE_STATUS] | string
97-97: Avoid using index signature with 'any' type in IPlugin.The index signature
[key: string]: anyin the IPlugin interface reduces type safety. Consider either removing it if all properties are known, or constraining the value type.interface IPlugin { id: string componentName?: string name?: string label?: string - [key: string]: any + // Add more specific properties or constrain the value type + [key: string]: string | number | boolean | undefined | object }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/layout/src/composable/useLayout.ts(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (22)
packages/layout/src/composable/useLayout.ts (22)
14-14: Good addition of the RemovableRef import type.Adding the type import for RemovableRef from @vueuse/core is a good practice for TypeScript, as it enhances type safety for the storage references used in the file.
196-198: Good usage of generic type parameters with useStorage.Using generic type parameters with useStorage is a good practice that enhances type safety and provides better IDE support. This ensures that the stored values are correctly typed.
212-214: Good usage of generic type parameters with useStorage for array types.Properly typing the arrays stored in localStorage using
useStorage<string[]>ensures type safety and provides better IDE support.
300-304: Improved error handling with TypeScript instanceof check.The improved error handling with a TypeScript instanceof check is a good addition. It provides more specific error handling and better error messages.
306-306: Good use of generic type for pluginStorageReactive.Using the generic type
IPluginStoragewith useStorage ensures type safety for the plugin storage object.
315-317: Good conditional assignment for optional offset parameter.The proper handling of the optional offset parameter with an undefined check is a good improvement.
103-141: Well-structured interface for the return type.The IUseLayoutReturn interface is well-structured and provides a clear contract for the function's return value. This improves code maintainability and documentation.
143-150: Good annotation of PLUGIN_POSITION with interface type.Adding the IPluginPosition type to the PLUGIN_POSITION constant improves type safety and documentation.
152-154: Good use of reactive with type annotation.Using the IPluginState interface with reactive() ensures type safety for the reactive object.
156-186: Good use of ILayoutState interface with reactive.The layoutState object is properly typed with the ILayoutState interface, which improves type safety and documentation.
188-194: Good addition of return types to getter and setter functions.Adding proper return type annotations to the getter and setter functions improves code readability and type safety.
199-210: Good typing of menuName parameter with union type.Using the union type 'left' | 'right' for the menuName parameter restricts the possible values to just these two options, which improves type safety.
215-225: Good addition of parameter and return types to panel manipulation functions.Adding proper parameter and return type annotations to the changeLeftFixedPanels and changeRightFixedPanels functions improves code readability and type safety.
227-232: Good type annotations for getter functions.Adding proper return type annotations to the getter functions improves code readability and type safety.
234-236: Good usage of Partial type for setDimension.Using Partial for the setDimension function parameter allows for updating only a subset of the dimension properties, which is a good TypeScript practice.
291-304: Properly typed default export function with return type.Adding the IUseLayoutReturn return type to the default export function improves type safety and documentation.
337-339: Well-typed getPluginById function.The getPluginById function is now properly typed with parameter and return types, which improves type safety and documentation.
341-343: Good typing for getPluginsByPosition function.The getPluginsByPosition function now has proper parameter and return types, including an array that may contain undefined values.
360-400: Good typing for dragPluginLayout function.The dragPluginLayout function, which is complex, now has proper parameter and return types, which improves type safety and documentation.
403-411: Well-typed isSameSide function.The isSameSide function now has proper parameter and return types, which improves type safety and documentation.
414-422: Typed getPluginShown and changePluginShown functions.These functions now have proper parameter and return types, which improves type safety and documentation.
429-434: Properly typed utility functions.The isPanelWidthResizable and initPluginStorageReactive functions now have proper parameter and return types, which improves type safety and documentation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/layout/src/composable/useLayout.ts (5)
261-266: Consider adding return type to closeSetting function.While most functions have been properly typed, closeSetting is missing a return type annotation.
-const closeSetting = (forceClose?: boolean) => { +const closeSetting = (forceClose?: boolean): void => { const { settings } = layoutState if (!settings.fixedPanels.includes(settings.render) || forceClose) { settings.render = '' } }
282-287: Consider adding return type to closePlugin function.While most functions have been properly typed, closePlugin is missing a return type annotation.
-const closePlugin = (forceClose?: boolean) => { +const closePlugin = (forceClose?: boolean): void => { const { plugins } = layoutState if (!plugins.fixedPanels.includes(plugins.render) || forceClose) { plugins.render = '' } }
289-289: Add return type to isEmptyPage function.The isEmptyPage function should explicitly return a boolean type.
-const isEmptyPage = () => layoutState.pageStatus?.state === PAGE_STATUS.Empty +const isEmptyPage = (): boolean => layoutState.pageStatus?.state === PAGE_STATUS.Empty
255-259: Consider adding type annotation to getFixedPanelsStatus function.While most functions have been properly typed, getFixedPanelsStatus is missing return type annotation.
-const getFixedPanelsStatus = () => { +const getFixedPanelsStatus = (): IFixedPanelsStatus => { const leftPanelFixed = leftFixedPanelsStorage.value.includes(layoutState.plugins.render) const rightPanelFixed = rightFixedPanelsStorage.value.includes(layoutState.settings.render) return { leftPanelFixed, rightPanelFixed } }
346-350: Consider adding proper return type to changePluginLayout.While most functions have been properly typed, changePluginLayout should explicitly return void.
-const changePluginLayout = (name: string, layout: string): void => { +const changePluginLayout = (name: string, layout: string): void => { if (pluginStorageReactive.value[name]) { pluginStorageReactive.value[name].align = layout } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/layout/src/composable/useLayout.ts(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (18)
packages/layout/src/composable/useLayout.ts (18)
14-14: Good addition of RemovableRef type import.Adding the RemovableRef type import from @vueuse/core ensures proper typing for the storage refs used throughout the file.
21-141: Well-structured type definitions improve code quality.The comprehensive type definitions added here provide excellent structure and clarity to the codebase. The interfaces appropriately model the various aspects of the layout system, making the code more maintainable and enabling better IDE support with autocomplete and type checking.
143-143: Good type annotation for PLUGIN_POSITION constant.Adding the IPluginPosition type to the PLUGIN_POSITION constant ensures type safety when accessing its properties.
152-154: Good type annotation for pluginState reactive object.The IPluginState type annotation on the pluginState reactive object enhances type safety.
156-186: Proper typing for layoutState reactive object.The ILayoutState type annotation on the complex layoutState reactive object improves type safety and provides better documentation of the object structure.
188-194: Well-typed getter and setter functions for MoveDragBarState.Adding boolean return type to getMoveDragBarState and boolean parameter type to changeMoveDragBarState improves type safety.
196-197: Good use of generic types for storage hooks.Using generic types with useStorage hooks (useStorage, useStorage<string[]>) ensures type safety for the stored values.
199-210: Properly typed changeMenuShown function.The MenuType union type ('left' | 'right') for the menuName parameter ensures that only valid values can be passed.
212-213: Good use of generic types for fixed panels storage.Using generic types with useStorage hooks for the fixedPanels arrays ensures type safety.
215-219: Well-typed panel modification functions.Adding proper parameter and return types to changeLeftFixedPanels and changeRightFixedPanels makes the API more predictable.
Also applies to: 221-225
227-236: Clear return types for getter and setter functions.Adding proper return types to getScale, getPluginState, getSettingState, getDimension, and parameter types to setDimension improves code clarity.
269-279: Proper Promise typing for activePlugin.The Promise return type is now properly typed with IMetaApi, ensuring type safety for consumers of this function.
291-304: Improved error handling with type checking.Good enhancement to the error handling with proper type checking using the
instanceof Errorcheck and more specific error messages.
306-306: Good use of generic type for plugin storage.Using the generic type with useStorage hook for pluginStorageReactive ensures type safety.
312-321: Fixed object assignment in changePluginWidth.The implementation now correctly initializes a new object with required properties when the plugin name doesn't exist in storage.
337-343: Well-typed utility functions for plugin retrieval.The getPluginById and getPluginsByPosition functions now have proper parameter and return type annotations, making the API more predictable.
360-400: Thorough type annotations for dragPluginLayout function.The complex dragPluginLayout function now has proper parameter types, improving code clarity and helping prevent potential bugs.
417-422: Fixed initialization in changePluginShown function.Now properly initializes a new plugin object with required properties when it doesn't exist in storage.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/layout/src/composable/useLayout.ts (1)
119-119:⚠️ Potential issueProperty pluginEvent not defined in IPlugins interface
The layoutState.plugins object contains a pluginEvent property, but this property is not defined in the IPlugins interface. This inconsistency could lead to type errors.
interface IPlugins { isShow: boolean fixedPanels: string[] render: string activating: boolean showDesignSettings: boolean + pluginEvent: 'all' | string }
🧹 Nitpick comments (4)
packages/layout/src/composable/useLayout.ts (4)
20-23: IMetaApi interface is overly permissiveThis interface uses
[key: string]: anywhich provides minimal type safety. Consider defining more specific properties if known, or adding documentation explaining why this generic structure is necessary.// MetaApi 类型定义 interface IMetaApi { [key: string]: any + // TODO: Add specific known properties to improve type safety }
34-36: Consider using more specific union type for pluginEventUsing
'all' | stringis functionally equivalent to juststringsince it allows any string value. If there are specific valid values for pluginEvent, consider listing them explicitly in the union.interface IPluginState { - pluginEvent: 'all' | string + pluginEvent: 'all' | 'specific1' | 'specific2' | string }
70-77: Remove redundantundefinedin optional propertiesThe
?operator already indicates that a property can be undefined, so adding| undefinedis redundant.interface IPluginStorageItem { - width?: number | undefined + width?: number offset?: number align?: string index: number isShow?: boolean widthResizable?: boolean }
175-175: Inconsistent return type annotation styleThe getScale function uses arrow function shorthand for return type, while other functions use explicit type annotations. For consistency, consider using the same style as other functions.
-const getScale = (): number => layoutState.dimension.scale +const getScale = (): number => { + return layoutState.dimension.scale +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/layout/src/composable/useLayout.ts(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (5)
packages/layout/src/composable/useLayout.ts (5)
248-251: Improved error handling with type checkingGood use of type checking to handle errors properly. This ensures errors are propagated with appropriate messages.
267-267: Object creation with default values is now correctThis implementation properly sets default values when creating a new plugin storage entry, addressing the issue mentioned in a previous review.
367-367: Object creation with default values is now correctThis implementation properly sets default values when creating a new plugin storage entry, addressing the issue mentioned in a previous review.
289-291: Good use of union type for potential undefined returnsThe function correctly types the return value as an array that may contain undefined elements
(IPlugin | undefined)[], which accurately represents the function's behavior.
384-422:❌ Incorrect review comment
Need explicit return type for object properties
The return object contains many methods and properties without type declarations. Consider adding type annotations to each method in the return object for maximum type safety.
🌐 Web query:
Is it necessary to declare types for properties in a return object in TypeScript, or will TypeScript infer these types correctly?💡 Result:
In TypeScript, explicit type annotations for return object properties are generally not necessary because the compiler can accurately infer their types from the returned values. However, there are scenarios where explicit typing becomes beneficial or necessary.
Key Inference Capabilities
Automatic type inference
TypeScript infers object property types based on initial values:function getUser() { return { id: 1, // inferred as number name: "Alice" // inferred as string }; // Return type: { id: number; name: string } }Const assertions
For literal values, useas constto preserve exact types:function getStatus() { return { code: 200, message: "OK" } as const; } // Inferred return type: { readonly code: 200; readonly message: "OK" }When Explicit Typing Helps
Scenario Example Benefit Multiple return branches function parse(input: string): { success: boolean; data?: string } { ... }Ensures consistent type across code paths [3][11] Library development export function createConfig(): Readonly<Config> { ... }Improves declaration file generation [3] Complex transformations type Transformed<T> = { [K in keyof T]: T[K] extends string ? Date : T[K] }Maintains intentional type relationships [4][13] Documentation clarity interface User { id: number; name: string }function getUser(): User { ... }Acts as built-in documentation [9][12] Inference Limitations
Mutable variables may widen types unintentionally:
function getCounter() { const count = 0; // inferred as number return { count, increment: () => count++ }; } // count remains number, but explicit annotation could enforce stricter numeric subtypesEmpty objects require explicit typing:
function createWidget(): Widget { return {}; // Error without Widget interface definition }For most application code, relying on type inference is preferred to reduce boilerplate while maintaining type safety. Explicit return type annotations become valuable in libraries, complex functions, or when enforcing specific contracts across teams[3][6].
Citations:
- 1: https://mayashavin.com/articles/types-from-constants-typescript
- 2: https://www.devoreur2code.com/blog/type-inference-with-typescript
- 3: https://www.totaltypescript.com/tips/dont-use-return-types-unless
- 4: https://nozzlegear.com/blog/modify-the-property-types-on-an-object-with-conditional-typing
- 5: https://blog.dennisokeeffe.com/blog/2023-06-13-understanding-advance-type-inference-in-typescript
- 6: https://engineering.brigad.co/understanding-typescript-type-inference-and-const-assertions-4879db1cb980
- 7: https://exploringjs.com/tackling-ts/ch_typing-objects.html
- 8: https://dev.to/mrcmesen/infer-types-from-objects-and-arrays-with-typescript-15km
- 9: https://davidgomes.com/annotate-vs-type-inference/
- 10: https://community.palantir.com/t/how-do-i-get-an-object-types-property-api-names-typescript/1506
- 11: https://www.typescriptlang.org/docs/handbook/type-inference.html
- 12: https://www.typescriptlang.org/docs/handbook/2/functions.html
- 13: https://www.typescriptlang.org/docs/handbook/utility-types.html
- 14: https://www.typescriptlang.org/docs/handbook/advanced-types.html
- 15: https://www.w3schools.com/typescript/typescript_object_types.php
- 16: feat: inference of object properties based on a "type" infer out microsoft/TypeScript#53883
- 17: https://www.typescriptlang.org/docs/handbook/2/objects.html
- 18: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html
Explicit Return Types Are Optional Here
Based on TypeScript’s strong type inference, the code in
packages/layout/src/composable/useLayout.ts(lines 384–422) correctly infers all object property types from their implementations. Explicit return type annotations aren’t necessary unless you need a clear contract for external consumers or additional documentation. If this module is internal or its API is clear through inference, the current approach is acceptable.Likely an incorrect or invalid review comment.
packages/layout/src/composable/index.ts 中有几个类型错误，原因是类型未导出。需要给useLayout中的类型加上export导出
|
There are several type errors in packages/layout/src/composable/index.ts because the type is not exported. You need to add export to the types in useLayout
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/layout/src/composable/useLayout.ts (1)
239-240: 🛠️ Refactor suggestionMissing return type definition for useLayout function
The main exported function useLayout lacks a return type annotation, which would complete the type safety of this module. Consider adding a return type that includes all the properties being returned.
-export default () => { +export default (): { + isPanelWidthResizable: (name: string) => boolean + getFixedPanelsStatus: () => { leftPanelFixed: boolean, rightPanelFixed: boolean } + initPluginStorageReactive: (pluginList: IPluginStorage) => void + PLUGIN_NAME: any + PLUGIN_POSITION: IPluginPosition + // Add remaining return properties... +} => {
🧹 Nitpick comments (5)
packages/layout/src/composable/useLayout.ts (5)
203-207: Missing return type for getFixedPanelsStatus functionThe
getFixedPanelsStatusfunction lacks a return type annotation, which would complete the type safety improvements in this file.-const getFixedPanelsStatus = () => { +const getFixedPanelsStatus = (): { leftPanelFixed: boolean, rightPanelFixed: boolean } => {
209-214: Missing return type for closeSetting functionThe
closeSettingfunction lacks a return type annotation.-const closeSetting = (forceClose?: boolean) => { +const closeSetting = (forceClose?: boolean): void => {
217-227: Improve return type annotation for activePlugin functionWhile the Promise return type is specified in the implementation, it would be clearer to include it in the function signature.
-const activePlugin = (name: string, noActiveRender: boolean) => { +const activePlugin = (name: string, noActiveRender: boolean): Promise<IMetaApi> => {
230-235: Missing return type for closePlugin functionThe
closePluginfunction lacks a return type annotation.-const closePlugin = (forceClose?: boolean) => { +const closePlugin = (forceClose?: boolean): void => {
237-237: Missing return type for isEmptyPage functionThe
isEmptyPagefunction lacks a return type annotation.-const isEmptyPage = () => layoutState.pageStatus?.state === PAGE_STATUS.Empty +const isEmptyPage = (): boolean => layoutState.pageStatus?.state === PAGE_STATUS.Empty
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/layout/src/composable/useLayout.ts(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (14)
packages/layout/src/composable/useLayout.ts (14)
20-89: Good addition of TypeScript interfacesThese interfaces provide clear structure and improve type safety throughout the codebase. The definitions are comprehensive and will help catch potential errors during development.
67-67: Avoid usinganytype for pageStatusUsing
anyloses type safety. Since pageStatus is accessed with a.stateproperty on line 237, consider creating a more specific interface for this property.- pageStatus: any + pageStatus: { + state: string + [key: string]: any + }
91-134: Good type annotations for constants and state objectsThe addition of type annotations to
PLUGIN_POSITION,pluginState, andlayoutStateimproves type safety and code readability.
136-142: Good type annotations for getMoveDragBarState and changeMoveDragBarStateThe addition of return types and parameter types to these functions improves code clarity and prevents potential errors.
144-146: Good use of generics with useStorageUsing generic types with
useStorageensures proper type checking of stored values.
147-173: Good type annotations for menu and panel functionsThe addition of parameter types and return types to
changeMenuShown,changeLeftFixedPanels, andchangeRightFixedPanelsimproves code clarity and prevents potential errors.
175-184: Good type annotations for getter and setter functionsThe addition of return types to
getScale,getPluginState,getSettingState,getDimension, andsetDimensionimproves code clarity and prevents potential errors.
247-252: Good improvement in error handlingThe error handling has been improved by checking if the error is an instance of Error before accessing its message property, and providing a default error message otherwise.
254-254: Good use of generic with useStorage for plugin storageUsing generic type with
useStorageensures proper type checking of stored plugin values.
257-273: Good type annotations for plugin width and layout functionsThe addition of return types and parameter types to
getPluginWidth,changePluginWidth, andgetPluginByLayoutimproves code clarity and prevents potential errors.
260-269: Good fix for the object assignment issueThe previous implementation had a potential issue with object assignment. This has been fixed by properly initializing the object with all necessary properties.
285-291: Good type annotations for plugin retrieval functionsThe addition of parameter types and return types to
getPluginByIdandgetPluginsByPositionimproves code clarity and prevents potential errors.
367-368: Good fix for null reference issueThe previous implementation had a potential issue when trying to assign properties to an object that doesn't exist yet. This has been fixed by properly initializing the object first.
308-348: Good type annotations for dragPluginLayout functionThe addition of parameter types and return type to
dragPluginLayoutimproves code clarity and prevents potential errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/layout/src/composable/useLayout.ts (4)
20-23: Interface definition for IMetaApi is too permissiveThe
IMetaApiinterface only has an index signature withanytype, which doesn't provide much type safety. Consider defining the expected properties and methods that this API should have, even if incomplete initially.export interface IMetaApi { - [key: string]: any + // Add known properties with specific types + // For example: + getPlugins?: () => IPlugin[] + // Maintain the index signature for unknown properties + [key: string]: any }
224-226: Promise return type is good, but lacks error handlingWhile adding the proper return type
Promise<IMetaApi>is good, there's no error handling ifgetMetaApifails.return new Promise<IMetaApi>((resolve) => { - nextTick(() => resolve(getMetaApi(name))) + nextTick(() => { + try { + resolve(getMetaApi(name)) + } catch (error) { + console.error(`Failed to get meta API for plugin ${name}:`, error) + resolve({} as IMetaApi) + } + }) })
248-252: Improved error handling but could be more specificThe error handling has been improved with proper type checking, but the error message for non-Error instances is generic.
if (error instanceof Error) { throw new Error(error.message) } else { - throw new Error('Failed to parse plugin storage') + throw new Error(`Failed to parse plugin storage: ${String(error)}`) }
239-423: Consider adding JSDoc comments to exported functionThe main exported function and its returned methods lack JSDoc comments explaining their purpose, parameters, and return values.
Adding JSDoc comments would improve the developer experience and documentation:
+/** + * Provides utilities for managing the layout of plugins and settings in the application. + * @returns An object containing methods and properties for layout management. + */ export default () => { // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/layout/src/composable/useLayout.ts(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (11)
packages/layout/src/composable/useLayout.ts (11)
67-67: Avoid usinganytype for pageStatusUsing
anyloses type safety. Since pageStatus is accessed with a.stateproperty on line 237, consider creating a more specific interface for this property.- pageStatus: any + pageStatus: { + state: string + [key: string]: any + }
70-77: Good type definition for IPluginStorageItemThe type definition for
IPluginStorageItemproperly captures all the properties used throughout the code with appropriate optionality markers. This will help prevent runtime errors when properties are accessed.
136-138: Good type safety for MoveDragBarState functionsAdding the boolean return type and parameter type to these functions improves type safety and makes the API more predictable.
Also applies to: 140-142
144-146: Good use of generic types with useStorageUsing generic types with
useStorageensures type safety when the stored values are accessed and modified throughout the code.
260-269: Properly handled type safety for changePluginWidth functionThe function now has proper type annotations and the conditional check for undefined is explicit, improving code safety and readability.
285-287: Well-defined plugin lookup function with proper return typeThe
getPluginByIdfunction now properly indicates it can returnundefinedwhen no matching plugin is found, making it safer to use throughout the codebase.
289-291: Improved type safety in getPluginsByPositionThe function's return type accurately reflects that each item could be undefined if a plugin is not found, which helps prevent runtime errors.
308-348: Properly typed complex dragPluginLayout functionThe complex dragPluginLayout function now has proper type annotations for parameters, making it easier to understand and use correctly.
366-368: Fixed initialization of plugin storage itemsThe code now properly initializes the plugin storage item as an object with the required properties, addressing the issue from previous reviews.
379-382: Type safe function to initialize plugin storageThe
initPluginStorageReactivefunction now has a proper type annotation for its parameter, ensuring that the expected data structure is provided.
239-240:❓ Verification inconclusive
Missing return type definition for useLayout function
The main exported function useLayout lacks a return type annotation, which would complete the type safety of this module.
-export default () => { +export interface IUseLayoutReturn { + isPanelWidthResizable: (name: string) => boolean + getFixedPanelsStatus: () => { leftPanelFixed: boolean, rightPanelFixed: boolean } + initPluginStorageReactive: (pluginList: IPluginStorage) => void + PLUGIN_NAME: any + PLUGIN_POSITION: IPluginPosition + activeSetting: (name: string) => void + closeSetting: (forceClose?: boolean) => void + activePlugin: (name: string, noActiveRender: boolean) => Promise<IMetaApi> + closePlugin: (forceClose?: boolean) => void + layoutState: ILayoutState + getScale: () => number + setDimension: (data: Partial<IDimension>) => void + getDimension: () => IDimension + getPluginById: (pluginList: IPlugin[], pluginId: string) => IPlugin | undefined + pluginState: IPluginState + getPluginState: () => IPlugins + getSettingState: () => ISettings + isEmptyPage: () => boolean + // Add remaining return properties... +} + +export default (): IUseLayoutReturn => {
🌐 Web query:
Are explicit return type interfaces recommended in Vue 3 composition API functions or is type inference sufficient?💡 Result:
In Vue 3's Composition API, Type Inference is generally sufficient for most use cases due to its robust integration with TypeScript, but explicit return type interfaces are recommended in specific scenarios for enhanced type safety and clarity.
Type Inference Strengths
Automatic type detection:
Composition API's reactivity primitives (ref,reactive,computed) infer types automatically[1][6][13]. For example:const count = ref(0) // Inferred as Ref<number> // No explicit type neededTemplate ref auto-unwrapping:
Ref values in templates are automatically unwrapped with correct type inference[16].Composable arguments:
Function parameters in composables get inferred types when using TypeScript[15].When to Use Explicit Return Types
Scenario Example Benefit Complex return objects interface AuthState { user: Ref<User>, token: Ref<string> }function useAuth(): AuthState { ... }Prevents fragmented type declarations[11][15] Union/intersection types function useToggle(): [Ref<boolean>, () => void]Avoids array-to-tuple conversion issues[4][15] Public composables export function usePagination<T>(): PaginationResult<T>Ensures contract stability for consumers[15] Conditional returns function useFetch<T>() => { data: Ref<T | null>, error: Ref<Error> }Clarifies branching logic outcomes[4] Performance & Best Practices
Runtime vs Type-based declarations:
Type-based props via generics (defineProps<{...}>()) have better inference than runtime declarations usingPropType[1][5][11].Computed properties:
Explicit generics prevent inference errors:const double = computed<number>(() => ...) // Enforce numeric returnIDE benefits:
Explicit types improve autocomplete and type checking in IDEs like VSCode[15].While Vue 3's Composition API reduces boilerplate through effective type inference[2][6][13], explicit return types become valuable in complex scenarios and public APIs. The official Vue documentation recommends leveraging TypeScript's inference where possible, while using explicit typing for shared libraries and critical business logic[1][3][5].
Citations:
- 1: https://vuejs.org/guide/extras/composition-api-faq.html
- 2: https://vueschool.io/articles/vuejs-tutorials/from-vue-js-options-api-to-composition-api-is-it-worth-it/
- 3: https://vuejs.org/guide/typescript/overview
- 4: https://www.nickyt.co/blog/infer-types-to-avoid-explicit-types-1pnn
- 5: https://vuejs.org/guide/typescript/composition-api
- 6: https://techhub.iodigital.com/articles/vue3-composition-api-in-a-nutshell
- 7: https://www.bizstream.com/blog/exploring-vue-3s-composition-api-and-performance-improvements/
- 8: https://www.koderhq.com/tutorial/vue/typescript/
- 9: https://www.vuemastery.com/blog/understanding-vue-3-expose/
- 10: https://www.tatvasoft.com/outsourcing/2024/11/vue-composition-api.html
- 11: https://madewithlove.com/blog/typescript-vue-3-and-strongly-typed-props/
- 12: https://antfu.me/posts/composable-vue-vueday-2021
- 13: https://vuejs.uz/guide/typescript/composition-api.html
- 14: https://dev.to/jacobandrewsky/understanding-the-vue-3-composition-api-fa2
- 15: https://dev.to/delia_code/the-ultimate-guide-to-vue-3-composition-api-tips-and-best-practices-54a6
- 16: https://www.thisdot.co/blog/vue-3-composition-api-ref-and-reactive
- 17: https://v3.ru.vuejs.org/api/composition-api.html
- 18: explicit-function-return-type should not be recommended mightyiam/eslint-config-love#860
- 19: https://vuejs.org/api/composition-api-setup
- 20: https://www.brewblox.com/dev/decisions/20210502_composition
Explicit Return Type for UseLayout Composable
Although Vue 3’s Composition API offers robust type inference, explicit return type annotations are recommended for public composables—especially when returning complex objects—to ensure clarity and maintainability. In this case, defining an explicit interface for the return value of
useLayoutenhances the API contract and aligns with best practices.
- Define an interface (e.g.,
IUseLayoutReturn) specifying all properties and methods being returned.- Annotate the exported function with this interface to signal its public API clearly.
Revised diff snippet:
-export default () => { +export interface IUseLayoutReturn { + isPanelWidthResizable: (name: string) => boolean; + getFixedPanelsStatus: () => { leftPanelFixed: boolean; rightPanelFixed: boolean }; + initPluginStorageReactive: (pluginList: IPluginStorage) => void; + PLUGIN_NAME: any; + PLUGIN_POSITION: IPluginPosition; + activeSetting: (name: string) => void; + closeSetting: (forceClose?: boolean) => void; + activePlugin: (name: string, noActiveRender: boolean) => Promise<IMetaApi>; + closePlugin: (forceClose?: boolean) => void; + layoutState: ILayoutState; + getScale: () => number; + setDimension: (data: Partial<IDimension>) => void; + getDimension: () => IDimension; + getPluginById: (pluginList: IPlugin[], pluginId: string) => IPlugin | undefined; + pluginState: IPluginState; + getPluginState: () => IPlugins; + getSettingState: () => ISettings; + isEmptyPage: () => boolean; + // Add any additional return properties as needed... +} + +export default (): IUseLayoutReturn => {This explicit typing not only clarifies the module's interface for consumers but also aids in effective IDE support and type checking.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/layout/src/composable/useLayout.ts (1)
20-23: Consider providing a more specific IMetaApi interface.The current
IMetaApiinterface is very loose with[key: string]: any. Consider defining more specific properties if they are known to provide better type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/layout/src/composable/useLayout.ts(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (11)
packages/layout/src/composable/useLayout.ts (11)
20-92: Good addition of type interfaces.The introduction of clear TypeScript interfaces for the layout system components provides better type safety and code documentation. These interfaces will help catch errors at compile time rather than runtime.
69-69: Replaceanytype with a more specific interface.Using
anyloses type safety. SincepageStatusis accessed with a.stateproperty elsewhere, consider creating a more specific interface.- pageStatus: any + pageStatus: { + state: string + [key: string]: unknown + }
102-102: Good use of generic type parameter with reactive.The addition of the generic type parameter
<IPluginState>to the reactive call ensures proper type checking of the reactive object properties.
146-148: Excellent use of generic types with useStorage.Adding generic type parameters to useStorage calls provides better type safety for stored values.
226-226: Good Promise typing.Adding the return type
Promise<IMetaApi>to the promise ensures that consumers of this function know what type to expect when the promise resolves.
241-242: Missing return type definition for useLayout function.The main exported function lacks a return type annotation, which would complete the type safety of this module.
-export default () => { +interface IUseLayoutReturn { + isPanelWidthResizable: (name: string) => boolean + getFixedPanelsStatus: () => { leftPanelFixed: boolean, rightPanelFixed: boolean } + initPluginStorageReactive: (pluginList: IPluginStorage) => void + PLUGIN_NAME: any + PLUGIN_POSITION: IPluginPosition + activeSetting: (name: string) => void + closeSetting: (forceClose?: boolean) => void + activePlugin: (name: string, noActiveRender?: boolean) => Promise<IMetaApi> + closePlugin: (forceClose?: boolean) => void + layoutState: ILayoutState + getScale: () => number + setDimension: (data: Partial<IDimension>) => void + getDimension: () => IDimension + getPluginById: (pluginList: IPlugin[], pluginId: string) => IPlugin | undefined + pluginState: IPluginState + getPluginState: () => IPlugins + getSettingState: () => ISettings + isEmptyPage: () => boolean + getPluginWidth: (name: string) => number + changePluginWidth: (name: string, width: number, offset?: number) => void + leftFixedPanelsStorage: Ref<string[]> + rightFixedPanelsStorage: Ref<string[]> + leftMenuShownStorage: Ref<boolean> + rightMenuShownStorage: Ref<boolean> + changeLeftFixedPanels: (pluginName: string) => void + changeRightFixedPanels: (pluginName: string) => void + getPluginsByLayout: (layout?: string) => string[] + changePluginLayout: (name: string, layout: string) => void + getPluginByLayout: (name: string) => string + dragPluginLayout: (from: string, to: string, oldIndex: number, newIndex: number) => void + isSameSide: (from: string, to: string) => boolean + getPluginShown: (name: string) => boolean + changePluginShown: (name: string) => void + changeMenuShown: (menuName: 'left' | 'right') => void + getMoveDragBarState: () => boolean + changeMoveDragBarState: (state: boolean) => void + getPluginsByPosition: (position: string, pluginList: IPlugin[]) => (IPlugin | undefined)[] +} + +export default (): IUseLayoutReturn => {
250-254: Improved error handling with TypeScript.The error handling has been enhanced with proper type checking for Error instances, which is a good practice.
265-267: Good use of optional parameter type check.The code now properly checks if the optional
offsetparameter is undefined before using it, which is safer than the previous version.
269-269: Fixed object initialization.The initialization of the plugin storage object now includes all required properties, addressing the previous review comment.
369-369: Fixed object initialization.The initialization of the plugin storage object now includes all required properties, addressing the previous review comment.
72-92:✅ Verification successful
Consider exporting more interfaces for external use.
Based on the PR comments from gene9831, you might need to export additional interfaces to resolve type errors in other files that import from this module.
🏁 Script executed:
#!/bin/bash # Check for interfaces and their imports/exports rg "\binterface I\w+" --type ts packages/layout/srcLength of output: 936
Export Additional Interfaces for External Use – Resolved
The file now exports several interfaces beyond those in the initially shown snippet. In addition to
IPluginStorageItem,IPluginStorage, andIPlugin, the file also exports interfaces such asIMetaApi,IPluginPosition,IPluginState,IDimension,IPluginBase,IPlugins,ISettings, andILayoutState. These additional exports address the concerns raised by gene9831 regarding type errors in other modules.No further changes are needed in this regard.
SonyLeo
force-pushed
the
refactor/useLayout
branch
from
efab8aa to
21dd813
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/layout/src/composable/useLayout.ts (4)
69-69: Consider replacinganytype with a more specific interface.The
pageStatusproperty is usinganytype which loses type safety. Since it's accessed with a.stateproperty later (line 242), consider using a more specific interface.- pageStatus: any + pageStatus: { + state: string + data: Record<string, unknown> + }
253-256: Consider rethrowing the original error to preserve stack trace information.The current error handling creates a new error which loses the original stack trace. Consider rethrowing or including the original error as a cause.
try { const storedPlugin = localStorage.getItem('plugin') if (storedPlugin) { plugin = JSON.parse(storedPlugin) as IPluginStorage } } catch (error) { if (error instanceof Error) { - throw new Error(error.message) + throw error } throw new Error('Failed to parse plugin storage') }
244-244: Consider adding return type annotation to the main exported function.For better type safety and documentation, consider adding a return type annotation to the main exported function.
You can either:
- Let TypeScript infer the return type (current approach)
- Create a specific return type interface
According to past comments, gene9831 suggested letting TypeScript infer the type, which is a valid approach.
168-178: Consider refactoring similar functions to reduce duplication.The
changeLeftFixedPanelsandchangeRightFixedPanelsfunctions have nearly identical implementations and could be refactored into a single helper function.+const toggleFixedPanel = (storage: Ref<string[]>, pluginName: string): void => { + storage.value = storage.value?.includes(pluginName) + ? storage.value?.filter((item) => item !== pluginName) + : [...storage.value, pluginName] +} + const changeLeftFixedPanels = (pluginName: string): void => { - leftFixedPanelsStorage.value = leftFixedPanelsStorage.value?.includes(pluginName) - ? leftFixedPanelsStorage.value?.filter((item) => item !== pluginName) - : [...leftFixedPanelsStorage.value, pluginName] + toggleFixedPanel(leftFixedPanelsStorage, pluginName) } const changeRightFixedPanels = (pluginName: string): void => { - rightFixedPanelsStorage.value = rightFixedPanelsStorage.value?.includes(pluginName) - ? rightFixedPanelsStorage.value?.filter((item) => item !== pluginName) - : [...rightFixedPanelsStorage.value, pluginName] + toggleFixedPanel(rightFixedPanelsStorage, pluginName) }Note: You'll need to import
Reffrom 'vue' for this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/layout/src/composable/useLayout.ts(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (9)
packages/layout/src/composable/useLayout.ts (9)
222-222: LGTM! Properly typed parameter with optional flag.The
noActiveRenderparameter is now correctly typed as an optional boolean parameter.
102-102: LGTM! Properly typed reactive state with generics.Using generics with reactive enables TypeScript to strictly validate the data structure.
271-273: LGTM! Fixed object assignment issue.The code properly initializes a new object with required properties when the plugin name is not found in storage.
371-373: LGTM! Fixed null reference issue.The code now properly initializes a new object with required properties when the plugin name doesn't exist.
290-296: LGTM! Well-typed helper functions for plugin retrieval.These functions properly use TypeScript types to ensure type safety when working with plugin data.
259-259: LGTM! Proper generic typing for storage.Using generics with
useStorageimproves type safety for stored values.
149-150: LGTM! Well-typed storage variables.The storage variables are properly typed with generics to ensure type safety.
93-100: LGTM! Well-typed constant with interface.The
PLUGIN_POSITIONconstant is now properly typed with theIPluginPositioninterface.
20-92:✅ Verification successful
LGTM! Well-structured interface definitions.
The TypeScript interface definitions are well-structured and provide clear type information for the data structures used throughout the code.
Based on the PR comments about VS Code errors, let's verify that necessary types are properly exported:
🏁 Script executed:
#!/bin/bash # Check if there are any references to these interfaces in other files rg -A 1 -B 1 "IPluginStorageItem|IPluginStorage|IPlugin" --type tsLength of output: 4564
Verified: Interface Exports Are Correct
The interface definitions in
packages/layout/src/composable/useLayout.tsare well-structured and properly exported. The grep search confirms that interfaces likeIPluginStorageItem,IPluginStorage, andIPluginare referenced in other parts of the codebase (e.g., inpackages/vue-generator/src/index.d.ts), which indicates that the VS Code errors related to type exports have been resolved.LGTM!
SonyLeo
force-pushed
the
refactor/useLayout
branch
from
21dd813 to
3f249b1
Compare
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
useLayout 增加类型声明
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit