feat(loading): [loading] added type definition and interface of Loading component to optimize loading style processing #3253
Conversation
…ng component to optimize loading style processing
WalkthroughThe changes update the component library to include mobile-first support for Skeleton components and expand the available type exports in Vue common adapters by adding the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Loadings as LoadingPlugin
participant Service as LoadingService
App->>Loadings: install(App)
Loadings->>Service: Initialize loading with configs
Service-->>Loadings: Return LoadingInstance
Loadings->>App: Attach loading instance to app root
Suggested labels
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
WARN GET http://10.0.0.28:4873/@antfu%2Feslint-config error (503). Will retry in 10 seconds. 2 retries left. This error happened while installing a direct dependency of /tmp/eslint ✨ Finishing Touches
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/modules.json(2 hunks)packages/vue-common/src/adapter/index.ts(1 hunks)packages/vue-common/src/adapter/vue2/index.ts(1 hunks)packages/vue-common/src/adapter/vue3/index.ts(1 hunks)packages/vue-common/src/index.ts(1 hunks)packages/vue/src/loading/index.ts(2 hunks)packages/vue/src/loading/src/service.ts(4 hunks)
🔇 Additional comments (18)
packages/vue-common/src/adapter/index.ts (1)
11-12: Export of App type added correctly.The addition of the
Apptype to the exports list follows the established pattern and makes this essential Vue type available to consumers of the library. This aligns with the PR objective of enhancing type definitions for the Loading component.packages/vue-common/src/adapter/vue3/index.ts (1)
496-497: App type export added for Vue 3 compatibility.The addition of the
Apptype to the Vue 3 adapter follows the same pattern as other type exports and maintains consistency with the changes in the other adapter files. This type is essential for components that need to interact with the Vue application instance.packages/vue-common/src/index.ts (1)
380-381: App type properly re-exported from main entry point.The addition of the
Apptype to the main exports ensures it's available throughout the library. This change is consistent with the pattern established for other Vue types and properly exposes the type to external consumers.packages/vue-common/src/adapter/vue2/index.ts (1)
356-357: App type export added for Vue 2 compatibility.The addition of the
Apptype from the Composition API package ensures compatibility for Vue 2 applications. This completes the consistent export of this type across all adapter interfaces, providing uniform type support regardless of the Vue version used.packages/modules.json (4)
2240-2241: Enhancement to Skeleton component with mobile-first support.Adding the "mobile-first" mode to the Skeleton component is a good improvement for responsive design, allowing the component to be optimized for mobile devices while maintaining PC support.
2249-2250: Enhancement to SkeletonItem component with mobile-first support.Similar to the parent Skeleton component, adding mobile-first support to SkeletonItem ensures consistent responsive behavior across related components.
2253-2257: Addition of SkeletonItemMobileFirst template component.The new component entry follows the established pattern in the codebase for mobile-first templates, providing the necessary template implementation for the mobile-first mode added to SkeletonItem.
2263-2267: Addition of SkeletonMobileFirst template component.This completes the mobile-first implementation for the Skeleton component family, adding the corresponding template for the main Skeleton component's mobile-first mode.
packages/vue/src/loading/index.ts (7)
16-17: Added type imports for improved type safety.Importing the App type from vue-common and ILoadingState from vue-renderless enhances type checking and provides better development experience.
19-31: Well-structured LoadingConfig interface.The new LoadingConfig interface properly defines all configuration options for the loading component with appropriate types, improving type safety and documentation.
33-39: Clearly defined LoadingInstance interface.This interface properly defines the structure of a loading instance, including its state, DOM element reference, and positioning properties.
41-56: Comprehensive type definitions for loading service.The LoadingService type and LoadingPlugin interface definitions provide clear contracts for how the loading functionality should be used, improving developer experience and preventing errors.
58-65: Type-safe initialization of loading service.The setupComponent.TINYLoading implementation with proper typing ensures the loading service is correctly attached to the root object. The ts-expect-error is properly documented to explain the runtime behavior.
67-69: Type-safe Loadings plugin definition.Updating the Loadings constant to use the LoadingPlugin interface ensures type safety when it's used and installed.
79-80: Appropriate handling of window.Vue type uncertainty.The @ts-expect-error comment properly acknowledges that window.Vue could be either Vue2 or Vue3, and that type compatibility will be handled at runtime.
packages/vue/src/loading/src/service.ts (3)
61-63: Type-safe function signature for addStyle.Updating the addStyle function to use typed parameters improves type safety and code clarity.
69-71: Improved error handling with optional chaining.The use of optional chaining (
?.) when accessing properties on potentially undefined objects is a good practice to prevent runtime errors.Also applies to: 79-81, 86-87
131-132:❓ Verification inconclusive
Type assertion for createComponent result.
The type assertion for the createComponent result ensures type safety for the instance object. Consider adding runtime validation to ensure the created component matches the expected interface.
🏁 Script executed:
#!/bin/bash # Check for usage of createComponent in the codebase echo "Checking createComponent usage patterns in the codebase to ensure consistent typing..." grep -r "createComponent" --include="*.ts" --include="*.js" packages/vue/src/ | head -n 20Length of output: 1357
Action: Consider Optional Runtime Validation for Component Instances
The type assertion using
as LoadingInstanceprovides static type safety, but it doesn't verify at runtime that the created component conforms to the expected interface. Our grep search across similar usages (e.g., in packages/vue/src/drawer and elsewhere) shows that runtime validation isn’t currently implemented anywhere. Please review whether adding runtime checks would enhance robustness in scenarios where the component might not fully meet the interface expectations.
- File: packages/vue/src/loading/src/service.ts (line 131-132)
}) as LoadingInstance- Suggestion: If runtime discrepancies are a concern, consider integrating runtime validation (possibly via a dedicated validation utility or library) to confirm the component’s structure conforms to the
LoadingInstanceinterface.
| export default (configs: LoadingConfig = {}) => { | ||
| configs = { ...defaults, ...configs } | ||
|
|
||
| if (typeof configs.target === 'string') { | ||
| configs.target = document.querySelector(configs.target) | ||
| configs.target = document.querySelector(configs.target) as HTMLElement | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unsafe type assertion for querySelector result.
The type assertion as HTMLElement on line 101 could be unsafe if the selector doesn't match any element, as it would assert null as HTMLElement.
Consider a more defensive approach:
- configs.target = document.querySelector(configs.target) as HTMLElement
+ const targetElement = document.querySelector(configs.target)
+ if (!targetElement) {
+ console.warn(`[Loading] Target element not found: ${configs.target}`)
+ }
+ configs.target = targetElement as HTMLElement📝 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.
| export default (configs: LoadingConfig = {}) => { | |
| configs = { ...defaults, ...configs } | |
| if (typeof configs.target === 'string') { | |
| configs.target = document.querySelector(configs.target) | |
| configs.target = document.querySelector(configs.target) as HTMLElement | |
| } | |
| export default (configs: LoadingConfig = {}) => { | |
| configs = { ...defaults, ...configs } | |
| if (typeof configs.target === 'string') { | |
| const targetElement = document.querySelector(configs.target) | |
| if (!targetElement) { | |
| console.warn(`[Loading] Target element not found: ${configs.target}`) | |
| } | |
| configs.target = targetElement as HTMLElement | |
| } | |
| // ... rest of the code | |
| } |
| import type { ILoadingState } from '@opentiny/vue-renderless/types/loading.type' | ||
|
|
||
| interface LoadingConfig { | ||
| text?: string | null | ||
| body?: boolean | ||
| lock?: boolean | ||
| customClass?: string | ||
| fullscreen?: boolean | ||
| iconSize?: string | ||
| target?: HTMLElement | string | ||
| size?: string | ||
| loadingImg?: string | ||
| tiny_mode?: string | ||
| } | ||
|
|
||
| interface LoadingInstance { | ||
| state: ILoadingState | ||
| $el: HTMLElement | ||
| originalPosition?: string | ||
| originalOverflow?: string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate interface definitions between files.
The LoadingConfig and LoadingInstance interfaces are defined both here and in index.ts. Consider moving these to a shared types file to avoid duplication and potential inconsistencies.
// Create a new file: packages/vue/src/loading/types.ts
+export interface LoadingConfig {
+ text?: string | null
+ body?: boolean
+ lock?: boolean
+ customClass?: string
+ fullscreen?: boolean
+ iconSize?: string
+ target?: HTMLElement | string
+ size?: string
+ loadingImg?: string
+ tiny_mode?: string
+}
+
+export interface LoadingInstance {
+ state: ILoadingState
+ $el: HTMLElement
+ originalPosition?: string
+ originalOverflow?: string
+}Then import these types in both files:
// In index.ts and service.ts
+import type { LoadingConfig, LoadingInstance } from './types'Committable suggestion skipped: line range outside the PR's diff.
增加 Loading 组件的类型定义和接口，优化加载样式处理
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
New Features
Refactor