fix(mobile): fix mobile components build error #2696
Conversation
WalkthroughThis pull request involves comprehensive changes to the mobile package's component structure and dependencies. The modifications primarily focus on updating import paths, removing stylesheet imports, adjusting component configurations, and streamlining the Vue common adapter. A new mobile theme dependency has been added to the package, and several utility functions have been introduced or reorganized in the Vue common adapter to improve component management and interaction. Changes
Suggested Labels
Poem
Possibly Related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
|
[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: 1
🧹 Nitpick comments (4)
packages/mobile/vue-common/src/adapter/index.ts (4)
25-33: Consider optional chaining for safer property accessIn
renderComponent, you might simplify the check forview?.value. For instance:- return () => hooks.h((view && view.value) || component, { ref: 'modeTemplate', ...props, ...attrs, ...extend }, slots) + return () => hooks.h(view?.value ?? component, { ref: 'modeTemplate', ...props, ...attrs, ...extend }, slots)This shortens code and avoids manual ternary or logical AND checks.
🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
64-69: Use optional chaining to simplify the route extractionHere, instead of:
const route = router && router.currentRoute.valueyou could consider optional chaining:
- const route = router && router.currentRoute.value + const route = router?.currentRoute?.valueThis clarifies the intention without manual checks.
79-90: Refine event broadcasting logic with optional chainingMultiple checks such as
vm.subTree && vm.subTree.children,child.type && child.type.componentName, andcomponent.$emitter && component.$emitter.emit(...)can be converted to optional chaining. For example:- const children = (vm.subTree && vm.subTree.children) || vm.children + const children = vm.subTree?.children || vm.children - const name = child.type && child.type.componentName + const name = child.type?.componentName - component.$emitter && component.$emitter.emit(eventName, params) + component.$emitter?.emit(eventName, params)This relies on short-circuit checks and clarifies the flow.
🧰 Tools
🪛 Biome (1.9.4)
[error] 79-82: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 84-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 88-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
303-364: Consider splitting thetoolsfunction for better maintainabilityThe
toolsfunction provides routing, i18n, event dispatching, parent/child references, and more. Splitting it into smaller, dedicated helper functions (e.g., separate route logic, separate parent attribute handling) can make the code more understandable and testable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/mobile/components/container/index.ts(0 hunks)packages/mobile/components/date-picker/src/mobile.vue(1 hunks)packages/mobile/components/loading/index.ts(1 hunks)packages/mobile/components/modal/index.ts(1 hunks)packages/mobile/components/multi-select-item/src/mobile.vue(1 hunks)packages/mobile/components/popover/index.ts(0 hunks)packages/mobile/components/tooltip/src/mobile.vue(1 hunks)packages/mobile/components/upload/src/mobile.vue(1 hunks)packages/mobile/package.json(1 hunks)packages/mobile/vite.config.ts(1 hunks)packages/mobile/vue-common/index.ts(1 hunks)packages/mobile/vue-common/src/adapter/index.ts(2 hunks)packages/mobile/vue-common/src/adapter/vue3/index.ts(0 hunks)packages/mobile/vue-common/src/adapter/vue3/package.json(0 hunks)
💤 Files with no reviewable changes (4)
- packages/mobile/components/popover/index.ts
- packages/mobile/vue-common/src/adapter/vue3/package.json
- packages/mobile/components/container/index.ts
- packages/mobile/vue-common/src/adapter/vue3/index.ts
✅ Files skipped from review due to trivial changes (2)
- packages/mobile/components/tooltip/src/mobile.vue
- packages/mobile/components/modal/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/mobile/vue-common/src/adapter/index.ts
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 68-71: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 79-82: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 84-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 88-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 100-100: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 105-107: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 112-113: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 120-120: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 218-218: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 240-242: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 268-269: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 458-461: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (12)
packages/mobile/vue-common/src/adapter/index.ts (3)
1-11: License block looks good
All relevant legal notices are clearly included. No issues with the license header and copyright.
420-441: Ensure custom element detection aligns with all supported browsers
When creating custom elements dynamically, ensure that the document.createElement approach for detection remains valid for all targeted user agents. If your mobile environment includes older browsers, consider polyfills or alternative checks for custom elements (e.g., customElements.get(...)) if needed.
477-487: Validate definitions and usage for newly exported composition APIs
defineComponent, isVue2, isVue3, isVnode, etc., are being exported for use across mobile components. Confirm that internal references to these exports in related files (like plugin setups or external usage) reflect the correct version checks and do not conflict with existing Vue 2 or Vue 3 code.
packages/mobile/vite.config.ts (2)
13-13: Expanded External Dependencies
Including 'vue' and 'xss' in external properly excludes them from the bundle. If these dependencies are used internally, ensure that all references are accounted for and that this aligns with your final distribution strategy.
7-7: Confirm Removal of dts Plugin
It appears the dts plugin was removed from the build. Verify there are no remaining references to it (e.g., in package.json, scripts, or other config files) to prevent potential build or dependency issues.
packages/mobile/components/loading/index.ts (1)
14-14: Consistent Import Path Update
Changing the import path from ../../../vue-common to ../../vue-common maintains consistency with other components in this PR. Verify that any references or relative paths in related modules (e.g., modal/index.ts) use the correct level of directory traversal.
✅ Verification successful
Let me analyze the search results to verify the import path change.
The search results show that vue-common is a core package used throughout the codebase, and it's imported in various ways:
- As a workspace dependency in package.json files:
"@opentiny/vue-common": "workspace:~" - In source files using relative paths:
'../../vue-common' - As a direct import:
'@opentiny/vue-common'
Let me verify the specific file and its imports.
Let me check the actual content of the file and its surrounding structure to verify the import path change.
Let me check the directory structure to verify if the import path change is correct.
Import Path Change is Correct
The import path change from ../../../vue-common to ../../vue-common is correct. The directory structure shows that vue-common is located at packages/mobile/vue-common, and the loading component is at packages/mobile/components/loading/index.ts. Therefore, to reach vue-common from the loading component, going up two levels (../../) is the correct relative path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Confirm that all references to vue-common are consistent across the codebase.
rg -A 3 "vue-common"
Length of output: 66199
Script:
#!/bin/bash
# Check the directory structure around the file
fd loading packages/mobile/components
Length of output: 125
Script:
#!/bin/bash
# Check the content of the file and its directory
cat packages/mobile/components/loading/index.ts
ls -la packages/mobile/components/loading/
Length of output: 1186
Script:
#!/bin/bash
# Check the directory structure to verify the relative path
ls -la packages/mobile/components/
ls -la packages/mobile/vue-common 2>/dev/null || echo "vue-common not found at this level"
ls -la packages/mobile/ 2>/dev/null
Length of output: 4300
packages/mobile/components/date-picker/src/mobile.vue (1)
22-24: Improved Slot Clarity
Using <template #suffix> is a clean, explicit approach to defining the slot. This is a standard Vue convention and helps readability. Looks good!
packages/mobile/components/upload/src/mobile.vue (1)
14-14: Import Path Alignment
Importing defineComponent from the relative path ../../../vue-common appears consistent with other updates in this PR. Confirm that the file structure indeed places vue-common at this level, to avoid tricky import resolution errors.
✅ Verification successful
Let me verify the directory structure to ensure the import path is correct.
Import path is correctly aligned with the codebase structure
The import path ../../../vue-common is consistently used across all mobile components in the codebase. The extensive search results show that this is the standard import pattern used by all 60+ mobile component files for importing from vue-common, confirming that the import path in packages/mobile/components/upload/src/mobile.vue is correctly aligned with the codebase structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check references to `vue-common` in the mobile components upload directory
rg -A 2 "vue-common" packages/mobile/components/upload
Length of output: 310
Script:
#!/bin/bash
# Check if vue-common exists at the expected location
fd vue-common packages/mobile/components
# Check other files in the same directory for similar imports
rg "from.*vue-common" packages/mobile/components
Length of output: 8268
packages/mobile/vue-common/index.ts (1)
234-234: Consider verifying the removal of custom-class.
The destructuring of context.attrs no longer includes custom-class, which might break custom styling in certain scenarios. Ensure that any references to custom-class in parent components or templates have been updated or removed if no longer necessary.
packages/mobile/package.json (1)
35-35: Dependency addition looks good; confirm compatibility.
Adding @opentiny/vue-theme-mobile as a workspace dependency aligns with the theme-driven approach. However, confirm that this addition does not introduce version conflicts or unexpected behavior in existing components.
packages/mobile/components/multi-select-item/src/mobile.vue (2)
52-52: Validate relative import path usage for vue-common.
The import path '../../../vue-common' is consistent with the new structure, but ensure that any refactoring or folder reorganization does not break this relative import in the future.
55-55: Check alignment with new button import path.
Switching from @opentiny/vue-button to a relative path '../../button' may impact shared usage. Verify that the button component is correctly compiled and included, especially if other modules rely on the previous import.
| const parseProps = (propsData) => { | ||
| const props = {} | ||
|
|
||
| for (const name in propsData) { | ||
| if (name === 'class' || name === 'style') { | ||
| props[name] = propsData[name] | ||
| } else if (name === 'on' || name === 'nativeOn') { | ||
| const events = propsData[name] | ||
|
|
||
| for (const eventName in events) props[`on${capitalize(camelize(eventName))}`] = events[eventName] | ||
| } else if (name === 'attrs' || name === 'props' || name === 'domProps') { | ||
| const attrs = propsData[name] | ||
|
|
||
| for (const key in attrs) props[key] = attrs[key] | ||
| } else { | ||
| props[name] = propsData[name] | ||
| } | ||
| } | ||
|
|
||
| return props | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor nested property handling in parseProps
Your large conditional branches (lines 402–414) handle class, style, attrs, props, domProps, and events. Extracting these detail-specific transformations into smaller helper methods clarifies the code and makes future amendments simpler.
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
@opentiny/vue-theme-mobile.Bug Fixes
Documentation
Chores