Props hoc #3084
Props hoc #3084
Conversation
WalkthroughThis pull request introduces design configuration enhancements across components. The button demo now wraps Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CP as ConfigProvider
participant TR as TinyRow
participant TB as TinyButton
User->>CP: Initiate button interaction
CP->>TR: Wraps TinyButton(s)
TR->>TB: Supplies design settings (resetTime=0, size='mini')
TB->>User: Renders configured button
sequenceDiagram
participant BC as BaseComponent
participant HOC as WithDesignConfigPropsDefineComponent
participant GC as GlobalDesignConfig
participant Render as EnhancedComponent
BC->>HOC: Wrapped by HOC
HOC->>GC: Inject global design config
GC-->>HOC: Returns design properties
HOC->>Render: Merge props and render component
Render->>User: Display enhanced component
Suggested labels
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
examples/sites/demos/pc/app/button/basic-usage.vueOops! 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. examples/vue3/vite.config.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. internals/unplugin-hoc-plugin/src/esbuild.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
🪧 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 PR introduces a new feature by adding the Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| methods: { | ||
| // 解包可能存在的响应式对象 | ||
| unwrap(config) { | ||
| if (config && typeof config === 'object' && 'value' in config) { |
There was a problem hiding this comment.
Ensure that the unwrap method correctly handles all possible types of reactive objects to avoid unexpected behavior.
|
[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 |
| methods: { | ||
| // 解包可能存在的响应式对象 | ||
| unwrap(config) { | ||
| if (config && typeof config === 'object' && 'value' in config) { |
There was a problem hiding this comment.
Ensure that the unwrap method correctly handles all possible types of reactive objects to avoid unexpected behavior.
| // 获取组件级配置和全局配置（inject需要带有默认值，否则控制台会报警告） | ||
| let globalDesignConfig = hooks.inject(design.configKey, {}) | ||
| // globalDesignConfig 可能是响应式对象，比如 computed | ||
| globalDesignConfig = globalDesignConfig?.value || globalDesignConfig || {} |
There was a problem hiding this comment.
Ensure that the globalDesignConfig is correctly unwrapped to handle all possible types of reactive objects to avoid unexpected behavior.
![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 (3)
packages/vue-common/src/adapter/vue2/hoc.tsx (3)
14-28: Consider adding error handling for method exposureThe method exposure implementation watches for changes to
innerRefand copies methods to the wrapper instance. While functional, this approach could be more robust.Consider adding error handling to prevent potential issues when accessing methods:
created() { // 暴露实例方法 this.$watch( () => this.innerRef, (val) => { if (val) { Object.keys(val).forEach((key) => { if (!(key in this)) { + try { this[key] = val[key] + } catch (err) { + console.warn(`[DesignConfigHOC] Failed to expose method ${key}:`, err) + } } }) } } ) }
34-48: Consider adding component name to HOC for better debuggingThe HOC name is currently hardcoded as 'DesignConfigHOC', which may make debugging difficult when multiple components are wrapped.
Consider including the base component name in the HOC name:
export default function DesignConfigPropsHOC(BaseComponent) { + const componentName = BaseComponent.name || (BaseComponent.options && BaseComponent.options.name) || 'Unknown' return Vue.extend({ - name: 'DesignConfigHOC', + name: `DesignConfigHOC(${componentName})`, functional: false, // rest of the implementation...
50-57: Enhance unwrap method to handle more reactive typesThe current implementation may not handle all types of reactive objects in Vue ecosystem.
Consider expanding the unwrap method to handle more types of reactive objects:
unwrap(config) { if (config && typeof config === 'object') { // Handle Vue 2 reactivity if ('value' in config && typeof config.value !== 'undefined') { return config.value } + // Handle ref-like objects + if (typeof config.getValue === 'function') { + return config.getValue() + } } return config || {} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/sites/demos/pc/app/button/basic-usage.vue(3 hunks)examples/vue3/package.json(3 hunks)packages/vue-common/src/adapter/vue2.7/hoc.tsx(1 hunks)packages/vue-common/src/adapter/vue2.7/index.ts(2 hunks)packages/vue-common/src/adapter/vue2/hoc.tsx(1 hunks)packages/vue-common/src/adapter/vue2/index.ts(2 hunks)packages/vue-common/src/adapter/vue3/hoc.tsx(1 hunks)packages/vue-common/src/adapter/vue3/index.ts(2 hunks)packages/vue-common/src/index.ts(2 hunks)packages/vue/src/button/src/index.ts(1 hunks)
🔇 Additional comments (27)
packages/vue/src/button/src/index.ts (1)
12-12: Import updated to use WithDesignConfigPropsDefineComponentThe import statement has been modified to use the new
WithDesignConfigPropsDefineComponent(aliased asdefineComponent) instead of the directdefineComponentfrom@opentiny/vue-common. This change enables the Button component to support global design configurations.packages/vue-common/src/adapter/vue3/index.ts (2)
16-16: Added DesignConfigPropsHOC importThis import is required for the new
WithDesignConfigPropsDefineComponentfunction.
481-488: Added WithDesignConfigPropsDefineComponent functionThis new function implements a Higher-Order Component pattern to enhance components with design configuration capabilities. The implementation correctly wraps the component definition with the
DesignConfigPropsHOCafter processing it with Vue'sdefineComponent.The commented example code also provides good guidance on usage.
examples/sites/demos/pc/app/button/basic-usage.vue (4)
5-12: Added ConfigProvider to demonstrate design prop configurationThe basic buttons section has been wrapped with the new
ConfigProvidercomponent, demonstrating how to provide design configuration to button components. This serves as an excellent usage example for the new feature.
45-45: Added ConfigProvider to importsThe ConfigProvider component has been properly imported from the @opentiny/vue package.
52-54: Registered ConfigProvider in components objectConfigProvider has been correctly added to the components registration.
63-72: Added design configuration to component dataThis data property defines the design configuration that will be provided to all Button components within the ConfigProvider. The structure is well-organized with proper nesting for component-specific props.
examples/vue3/package.json (2)
15-15: Updated unit test command to use the UI interfaceThe unit test command now includes the
--uiflag to enable the Vitest UI interface, which provides a better testing experience.
40-40: Updated @vitest/ui dependency to a newer versionThe @vitest/ui package has been upgraded from ^0.31.0 to ^2.1.9, which is a significant version bump that provides newer features and bug fixes for the test UI interface.
packages/vue-common/src/adapter/vue2/index.ts (2)
6-6: Import addition looks goodThis correctly imports the
DesignConfigPropsHOCfrom the local HOC implementation file.
346-353: Well-structured HOC implementation with good documentationThe implementation of
WithDesignConfigPropsDefineComponentfollows the correct pattern for a higher-order component that wraps Vue's defineComponent. The commented example provides clear guidance on how to use this new function.A few observations:
- The function correctly takes a BaseComponent as input
- It properly composes the HOC with defineComponent
- TypeScript typing is correctly applied
- The example demonstrates the intended use case
This implementation will allow components to receive design configuration properties while maintaining compatibility with Vue's component definition API.
packages/vue-common/src/adapter/vue2.7/index.ts (2)
16-16: Import addition looks goodThis correctly imports the
DesignConfigPropsHOCfrom the Vue 2.7-specific HOC implementation.
345-352: Consistent implementation across Vue versionsThe implementation of
WithDesignConfigPropsDefineComponentfor Vue 2.7 matches the Vue 2 implementation, ensuring consistent behavior across Vue versions. The comment example is also helpful for developers.packages/vue-common/src/adapter/vue3/hoc.tsx (4)
1-4: Appropriate imports for Vue 3 HOCThe imports are well structured:
- Using Vue 3's type system with
SetupContext- Importing design utilities and hooks from the common index
- Importing the helper function
getComponentNamefrom the adapterThis sets up the necessary dependencies for the HOC implementation.
5-20: Well-designed global configuration injectionThe HOC implementation correctly:
- Preserves the original component's properties
- Sets up a ref for exposing the inner component
- Injects the global design configuration with a default empty object
- Handles potentially reactive configuration objects
- Retrieves component-specific configuration
- Merges configuration props with passed attrs
This approach ensures design configurations are properly injected while maintaining component functionality.
20-33: Effective method exposure through proxy patternThe implementation uses a Proxy to expose the inner component's methods, which is a clean approach to forward method calls and property access to the wrapped component. This maintains the component's public API.
34-43: Clean rendering implementation for Vue 3The render function correctly:
- Returns a JSX implementation of the BaseComponent
- Spreads the merged props to the component
- Passes the inner ref for method exposure
- Passes through slots
This completes the HOC pattern appropriately for Vue 3's composition API.
packages/vue-common/src/adapter/vue2.7/hoc.tsx (6)
1-3: Appropriate imports for Vue 2.7 HOCThe imports include Vue and the required design utilities while keeping the dependencies minimal.
4-14: Effective dependency injection setupThe HOC implementation correctly:
- Uses Vue.extend for Vue 2.7 compatibility
- Sets up dependency injection for the design configuration
- Provides a default empty object to prevent errors
This approach ensures consistent behavior with Vue 2.7's component model.
14-33: Clean method exposure patternThe created hook and watcher setup provide an elegant solution for exposing the inner component's methods to the outer component instance. The data initialization for innerRef is appropriately defined.
34-48: Well-designed computed propertiesThe computed properties correctly:
- Extract the component name by removing the prefix
- Handle the unwrapping of potentially reactive configuration objects
- Merge component-specific props with passed attributes
This ensures the design configuration is properly applied to the component.
49-57: Robust configuration handlingThe unwrap method properly handles different types of configuration objects, including reactive objects with a .value property. This ensures compatibility with both direct objects and computed properties.
58-73: Appropriate render implementation for Vue 2.7The render function correctly:
- Creates the BaseComponent with h function
- Sets up the ref callback to store the component instance
- Passes merged props as attrs
- Forwards listeners and slots
This completes the HOC pattern appropriately for Vue 2.7's rendering model.
packages/vue-common/src/index.ts (2)
24-24: LGTM: Import line updated to include WithDesignConfigPropsDefineComponentThe import now includes the new Higher-Order Component for design configuration, which aligns with the PR objective.
40-40: LGTM: Export updated to include WithDesignConfigPropsDefineComponentThe export correctly makes the new HOC available to other components in the application.
packages/vue-common/src/adapter/vue2/hoc.tsx (2)
4-73: Implementation of DesignConfigPropsHOC looks goodThis HOC effectively wraps components with design configuration capabilities, allowing for global design props to be merged with component-specific attributes.
The implementation:
- Correctly injects the global design configuration
- Computes component name by removing the prefix
- Merges component-specific design props with passed attributes
- Handles exposing inner component methods to the wrapper component
- Properly renders the base component with all necessary props and slots
58-71: LGTM: Render function implementationThe render function correctly passes all necessary props, listeners, and slots to the base component while setting up the reference for method exposure.
packages/vue/src/button/src/index.ts
Outdated
| @@ -9,7 +9,7 @@ | |||
| * A PARTICULAR PURPOSE. SEE THE APPLICABLE LICENSES FOR MORE DETAILS. | |||
| * | |||
| */ | |||
| import { $props, $prefix, $setup, defineComponent } from '@opentiny/vue-common' | |||
| import { $props, $prefix, $setup, WithDesignConfigPropsDefineComponent as defineComponent } from '@opentiny/vue-common' | |||
There was a problem hiding this comment.
如果这样修改可能会对现有业务造成影响，你这操作相当于包了一层组件转发下props，能不能通过某一个全局配置可以让用户自己选择是否开启这个功能，不要对存量业务造成影响
|
我看了下 vue-common 中的逻辑，目前组件应该都是两层组件的，可以在 vue-common 中统一处理第一层接收进来的 props， 我目前的思路是： 需要兼容之前的业务，可以全局增加公共 prop tiny_props_hoc，如果第一层组件传入了改 props，才执行替换逻辑，否则走原本逻辑，对原有业务不影响 |
I looked at the logic in vue-common. Currently, the components should be of two-layer components. The props received in the first layer can be uniformly processed in vue-common. My current thinking is:
It needs to be compatible with previous businesses, and you can add public prop tiny_props_hoc globally. If the first layer component passes in modified props, the replacement logic will be executed. Otherwise, the original logic will not affect the original business. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
internals/unplugin-virtual-template/src/core/transformVirtualTemplate.ts (4)
17-19: Typo in property name "tiny_experimetal_props_hoc"The property name contains a typo: "experimetal" instead of "experimental". This should be corrected for consistency and to avoid confusion. Search for this property name throughout the codebase to ensure all instances are updated.
- if(props.tiny_experimetal_props_hoc) { + if(props.tiny_experimental_props_hoc) {
35-39: Duplicate conditional logic for Props HOC featureThe conditional logic checking for
props.tiny_experimetal_props_hocis duplicated here and in lines 17-21. Consider extracting this to a helper function for better maintainability.// Add this helper function before the result declaration + const getImportGlob = (pattern, props) => { + if(props.tiny_experimental_props_hoc) { + return import.meta.glob(pattern, { eager: true, query: { withPropsHOC: true } }) + } else { + return import.meta.glob(pattern, { eager: true }) + } + } // Then use it in both places - if(props.tiny_experimetal_props_hoc) { - result = import.meta.glob('./{pc,mobile,mobile-first}.vue', { eager: true, query: { withPropsHOC: true } }) - } else { - result = import.meta.glob('./{pc,mobile,mobile-first}.vue', { eager: true }) - } + result = getImportGlob('./{pc,mobile,mobile-first}.vue', props)
46-47: Remove debugging console.log statementRemove the
console.log(props)statement as it shouldn't be included in production code. Consider using proper logging with log levels if debugging information is necessary.- console.log(props) -
30-40: Update function signature to match actual usageSince the
propsparameter is now required for the conditional logic, you should update the function signature to handle the case when it's not provided. This will prevent unexpected errors if the component is used without passing the props.-const ${localName} = (mode, props) => { +const ${localName} = (mode, props = {}) => { let result // #v-ifndef VITE_TINY_MODE const params = {} if(props.tiny_experimetal_props_hoc) { result = import.meta.glob('./{pc,mobile,mobile-first}.vue', { eager: true, query: { withPropsHOC: true } }) } else { result = import.meta.glob('./{pc,mobile,mobile-first}.vue', { eager: true }) }internals/unplugin-hoc-plugin/test/index.test.ts (2)
4-23: Add tests for edge cases in thetransformVirtualTemplatefunctionThe current tests don't cover edge cases such as how the function behaves when:
- The input doesn't match the expected format
- Different combinations of props values are provided
Consider adding tests for these scenarios to ensure robust behavior.
Example additional tests:
it('handles non-matching input correctly', () => { const input = "import template from 'some-other-import'"; expect(transformVirtualTemplate(input)).toBe(input); // Should return unchanged }); it('handles multiple virtual template imports in the same file', () => { const input = ` import template1 from 'virtual:template?pc' import template2 from 'virtual:template?mobile' `; const result = transformVirtualTemplate(input); expect(result.match(/const template\d+/g).length).toBe(2); });
25-43: Consider adding functional tests beyond snapshotsWhile snapshot tests are useful for detecting unexpected changes, they don't explicitly verify that the transformed code functions as expected. Consider adding tests that actually evaluate the generated code with various inputs to verify its runtime behavior.
Example approach for more complete testing:
it('generated function behaves correctly with different props', () => { // This is a more advanced test that would require: // 1. A mock for import.meta.glob // 2. A way to evaluate the generated function // 3. Assertions on the expected behavior // This might be better as an integration test that: // - Creates a temporary file // - Applies the transformation // - Uses the resulting code in a test Vue component // - Verifies the component behavior });patches/@opentiny__vue-vite-import@1.2.0.patch (1)
11-14: Improved module ID handling for import with query parametersThe patch correctly addresses the handling of module IDs with query strings by introducing
idWithoutQuery. This ensures that files with the?withPropsHOC=truequery parameter are properly processed.Consider using a more general approach to handle query strings in case other query parameters are introduced in the future:
-const idWithoutQuery = id.replace('?withPropsHOC=true', '') +const idWithoutQuery = id.split('?')[0]This would handle any query string, not just the specific
?withPropsHOC=trueparameter.internals/unplugin-hoc-plugin/src/types.ts (1)
1-18: Well-designed TypeScript type definitions for plugin optionsThe type definitions provide a clear contract for configuring the plugin:
Optionsinterface with well-documented defaults for include/exclude patternsUserOptionsas a partial ofOptionsallows for optional user configurationResolvedOptionsprovides type safety for the resolved configurationThe use of
FilterPatternfrom@rollup/pluginutilsis appropriate for defining include/exclude patterns, following the standard practice in Rollup/Vite plugins.Consider adding JSDoc comments for the
UserOptionsandResolvedOptionstypes to explain their purpose, similar to the documentation for properties in theOptionsinterface.internals/unplugin-hoc-plugin/src/index.ts (2)
14-21: Consider removing console.log commentsThe code contains commented out console.log statements that should be removed before merging to production.
- // console.log(filename, query) - if (filename.endsWith('.vue') && query) { - // console.log(id) + if (filename.endsWith('.vue') && query) { const params = new URLSearchParams(query) if (params.has('withPropsHOC')) { - // console.log('enter')
40-48: Consider adding documentation commentsThis plugin implements an important feature for props handling, but lacks documentation comments to explain the purpose and usage of the transformation.
export default createUnplugin<UserOptions>((options = {}) => { const filter = createFilter(options.include || ['**/*'], options.exclude || []) return { name: '@opentiny/unplugin-hoc-plugin', enforce: 'pre', transformInclude(id) { return filter(id) }, + /** + * Transforms Vue component files to use WithDesignConfigPropsDefineComponent. + * This enables props to be enhanced with design configuration values while maintaining backward compatibility. + * Only transforms files with the 'withPropsHOC' query parameter. + * + * @param code - Source code content + * @param id - File identifier with potential query parameters + * @returns Transformed code or null if no transformation needed + */ transform(code, id) {packages/vue-common/src/adapter/vue3/hoc.tsx (4)
18-19: Document the prop merging strategy.The current implementation merges designConfigProps with attrs, correctly prioritizing user-provided attributes over design configuration. Consider adding a comment to explicitly document this prioritization strategy for maintainability.
const designConfig = globalDesignConfig?.components?.[getComponentName().replace($prefix, '')] const designConfigProps = designConfig?.props || {} + // Priority: user-provided attributes (attrs) > design configuration props const mergedProps = { ...designConfigProps, ...attrs }
16-17: Add error handling for component name resolution.The current implementation assumes
getComponentName()will always return a valid string. Consider adding defensive code to handle cases where the component name might not be correctly determined.globalDesignConfig = globalDesignConfig?.value || globalDesignConfig || {} - const designConfig = globalDesignConfig?.components?.[getComponentName().replace($prefix, '')] + const componentName = getComponentName() + const designConfig = componentName ? globalDesignConfig?.components?.[componentName.replace($prefix, '')] : {} const designConfigProps = designConfig?.props || {}
46-52: Remove commented out code.The commented-out code provides an alternative implementation that's not being used. Consider removing it to improve code cleanliness and maintainability.
}) - // return () => { - // return ( - // <BaseComponent {...mergedProps} ref={innerRef}> - // {slots} - // </BaseComponent> - // ) - // } }
34-45:❓ Verification inconclusive
Consider directly passing slots to $setup.
The current implementation passes the entire context to $setup but then manually uses slots in the template function. Consider passing slots directly to the template for consistency with how they're used.
return $setup({ props, context, template: () => { return ( <BaseComponent {...mergedProps} ref={innerRef}> - {slots} + {Object.entries(slots).map(([name, slot]) => + slot && name === 'default' ? slot() : slot && <template slot={name}>{slot()}</template> + )} </BaseComponent> ) } })Let's check how slots are typically handled in other components:
🏁 Script executed:
#!/bin/bash # Search for slot usage patterns in existing components echo "Checking slot usage patterns in components..." rg -i "slots" --type tsx --context 5 | grep -A 5 "return ("Length of output: 188
Below is a new shell script to re-check the slot usage patterns while targeting the appropriate file type. It appears that searching for “tsx” didn’t work because that file type wasn’t recognized. This script uses the “typescriptreact” type (which covers TSX files) so we can see how slots are handled elsewhere.
Please run this script and verify if the slot usage in similar components follows the pattern of passing slots directly to
$setup(or via a similar standardized approach) rather than manually mapping them in a template. Once you have the output, we can finalize whether the suggested diff is consistent with the rest of the codebase.
🏁 Script executed:
#!/bin/bash echo "Re-checking slot usage patterns in components with typescriptreact file type..." rg -t typescriptreact "slots" --context 5 | grep -A 5 "return ("Length of output: 395
Action Required: Verify Consistent Slot Handling
The proposed diff changes how slots are passed by mapping them directly in the template instead of simply forwarding the context. However, our automated search for similar slot usage patterns in the repository was inconclusive. Please manually verify that this approach is consistent with slot handling in other components, and that it doesn’t inadvertently alter behavior elsewhere.
- File: packages/vue-common/src/adapter/vue3/hoc.tsx (lines 34–45)
- Suggestion: Instead of manually inserting
{slots}, update the code to directly pass slots according to the diff snippet below.return $setup({ props, context, template: () => { return ( <BaseComponent {...mergedProps} ref={innerRef}> - {slots} + {Object.entries(slots).map(([name, slot]) => + slot && name === 'default' ? slot() : slot && <template slot={name}>{slot()}</template> + )} </BaseComponent> ) } })Please review and confirm that this change aligns with how slots are managed throughout the project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internals/unplugin-hoc-plugin/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
examples/sites/demos/pc/app/button/basic-usage.vue(3 hunks)examples/vue3/package.json(3 hunks)examples/vue3/vite.config.ts(1 hunks)internals/unplugin-hoc-plugin/README.md(1 hunks)internals/unplugin-hoc-plugin/package.json(1 hunks)internals/unplugin-hoc-plugin/scripts/postbuild.ts(1 hunks)internals/unplugin-hoc-plugin/src/esbuild.ts(1 hunks)internals/unplugin-hoc-plugin/src/index.ts(1 hunks)internals/unplugin-hoc-plugin/src/nuxt.ts(1 hunks)internals/unplugin-hoc-plugin/src/rollup.ts(1 hunks)internals/unplugin-hoc-plugin/src/types.ts(1 hunks)internals/unplugin-hoc-plugin/src/vite.ts(1 hunks)internals/unplugin-hoc-plugin/src/webpack.ts(1 hunks)internals/unplugin-hoc-plugin/test/index.test.ts(1 hunks)internals/unplugin-hoc-plugin/tsconfig.json(1 hunks)internals/unplugin-hoc-plugin/tsup.config.ts(1 hunks)internals/unplugin-virtual-template/src/core/transformVirtualTemplate.ts(1 hunks)package.json(2 hunks)packages/vue-common/src/adapter/vue3/hoc.tsx(1 hunks)packages/vue-common/src/index.ts(5 hunks)packages/vue/src/button/src/index.ts(2 hunks)patches/@opentiny__vue-vite-import@1.2.0.patch(1 hunks)
✅ Files skipped from review due to trivial changes (10)
- internals/unplugin-hoc-plugin/src/webpack.ts
- internals/unplugin-hoc-plugin/src/esbuild.ts
- examples/vue3/vite.config.ts
- internals/unplugin-hoc-plugin/src/rollup.ts
- internals/unplugin-hoc-plugin/src/vite.ts
- internals/unplugin-hoc-plugin/tsup.config.ts
- internals/unplugin-hoc-plugin/README.md
- internals/unplugin-hoc-plugin/package.json
- packages/vue/src/button/src/index.ts
- internals/unplugin-hoc-plugin/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/sites/demos/pc/app/button/basic-usage.vue
- examples/vue3/package.json
- packages/vue-common/src/index.ts
🧰 Additional context used
🧬 Code Definitions (4)
internals/unplugin-hoc-plugin/test/index.test.ts (1)
internals/unplugin-virtual-template/src/core/transformVirtualTemplate.ts (1)
transformVirtualTemplate(1-54)
internals/unplugin-hoc-plugin/src/index.ts (1)
internals/unplugin-hoc-plugin/src/types.ts (1)
UserOptions(16-16)
packages/vue-common/src/adapter/vue3/hoc.tsx (4)
packages/vue-common/src/index.ts (5)
props(54-72)hooks(420-420)design(174-177)$prefix(42-42)$setup(136-171)packages/vue-common/src/adapter/vue2.7/index.ts (1)
getComponentName(42-51)packages/vue-common/src/adapter/vue2/index.ts (1)
getComponentName(53-62)packages/vue-common/src/adapter/vue3/index.ts (1)
getComponentName(42-51)
internals/unplugin-hoc-plugin/src/nuxt.ts (1)
internals/unplugin-hoc-plugin/src/types.ts (1)
UserOptions(16-16)
🔇 Additional comments (15)
internals/unplugin-hoc-plugin/scripts/postbuild.ts (1)
1-24: Well-structured post-build script for fixing CJS exportsThis script effectively addresses a common issue with CommonJS module exports when working with tools that generate both ESM and CJS outputs. It locates CJS files and modifies their export statements to ensure compatibility with both
importandrequirepatterns.The approach of replacing
exports.default =withmodule.exports =and appendingexports.default = module.exports;ensures dual compatibility with both:
- ES modules consumers:
import foo from 'package'- CommonJS consumers:
const foo = require('package')internals/unplugin-hoc-plugin/src/nuxt.ts (1)
1-16: Well-implemented Nuxt integration for the HOC pluginThe function correctly integrates the HOC plugin with both Webpack and Vite in a Nuxt environment.
Note the intentional difference in plugin placement:
- Webpack: Plugin is added to the beginning of the plugins array with
unshift()- Vite: Plugin is added to the end of the plugins array with
push()This approach is correct since plugin execution order often differs between Webpack and Vite, and this placement ensures proper operation in both environments.
patches/@opentiny__vue-vite-import@1.2.0.patch (1)
28-32: Fixed module identification for files with query parametersSimilar to the previous section, the patch introduces proper handling of module IDs with query strings.
package.json (2)
67-67: Added build script for HOC pluginA new build script has been added to manage the Higher-Order Component plugin, which aligns with the PR objective of implementing
WithDesignConfigPropsDefineComponentfunctionality.
157-158: New patched dependency addedA patch has been added for the
@opentiny/vue-vite-importpackage, which is likely needed to support the Higher-Order Component (HOC) functionality being implemented in this PR.internals/unplugin-hoc-plugin/src/index.ts (5)
1-4: LGTM! Import statements are appropriateThe imports include the necessary dependencies for creating an unplugin and handling file filtering.
5-13: Well-structured plugin initializationThe plugin is properly initialized with configurable include/exclude options and a clear name. The filter setup and transformInclude implementation follow best practices.
22-39: Import transformation looks correctThe code effectively transforms the import statements, replacing
defineComponentwithWithDesignConfigPropsDefineComponent as defineComponentfor Vue files with thewithPropsHOCquery parameter. This aligns with the PR objective to enable design configuration props handling.
1-49:❓ Verification inconclusive
Verify integration with other components
The PR objectives mention that only the Button component has been adjusted to use this method so far. Please ensure that this plugin correctly integrates with other components that will use this feature in the future.
🏁 Script executed:
#!/bin/bash # Check if any Vue components are already using the WithDesignConfigPropsDefineComponent echo "Searching for components that might already be using WithDesignConfigPropsDefineComponent..." rg "WithDesignConfigPropsDefineComponent" --type vue # Check the Button component implementation to verify correct usage echo "Checking Button component implementation..." fd "Button.vue" --type file | xargs rg -l "import.*from.*@opentiny/vue-common" | xargs catLength of output: 71861
Action Required: Verify Plugin Transformation in All Vue Components
The current verification steps did not yield conclusive evidence. The search for occurrences of “WithDesignConfigPropsDefineComponent” using “rg --type vue” failed due to an unrecognized file type, and the output from checking Button.vue only shows standard imports from “@opentiny/vue-common” without indication of the expected transformation.
Please manually verify (or use an alternative file search approach such as using “fd -e vue” with grep) the following:
- That components intended to use the “withPropsHOC” feature—beyond Button.vue—are receiving the correct import transformation (i.e., that “defineComponent” is replaced with “WithDesignConfigPropsDefineComponent as defineComponent”).
- That in a Vue file where the query parameter “withPropsHOC” is applied, the plugin’s transformation logic is executed and the resulting output reflects the intended changes.
1-49:❓ Verification inconclusive
Check for the HOC implementation
This plugin references
WithDesignConfigPropsDefineComponent, but we should verify that this function is properly implemented in the Vue 2 and Vue 3 adapters mentioned in the PR summary.
🌐 Web query:
What is a Higher-Order Component (HOC) in Vue?💡 Result:
Higher-Order Components (HOCs) in Vue.js are advanced design patterns for enhancing component reusability by wrapping components with extended functionality. Inspired by React's implementation but adapted for Vue's ecosystem, HOCs act as component factories that transform existing components into enhanced versions with added capabilities.
Core Mechanics
An HOC is a function that:
- Accepts a base component as input
- Returns a new component with:
- Additional props or state
- Modified template behavior
- Extended styling or lifecycle hooks
- New methods or computed properties
Key benefits include:
- Code reuse across multiple component variants
- Logic isolation for complex behaviors
- Non-invasive enhancement without modifying original components
- Structural consistency in UI patterns
Implementation Example
This HOC adds styling capabilities to any component:// withCustomStyle.js export default function withCustomStyle(Component, customStyle) { return { render(createElement) { return createElement(Component, { style: customStyle, }); } }; }Usage in components:
<script> import BaseComponent from './BaseComponent.vue'; import withCustomStyle from './withCustomStyle'; const EnhancedComponent = withCustomStyle(BaseComponent, { color: 'red', fontSize: '24px' }); export default { components: { EnhancedComponent } }; </script>Practical Applications
- UI Pattern Wrapping
Create consistent card variations while maintaining a single source of truth:<!-- ArticleCard.vue --> <template> <Card :border="'grey'"> <template #actions> <a :href="link">Read more</a> </template> </Card> </template> <!-- ProductCard.vue --> <template> <Card :border="'none'"> <template #actions> <BuyNowButton /> </template> </Card> </template>
- State Management
Share counter functionality across components using Composition API integration [5]:// WithCounter.js function WithCounter(WrappedComponent, incrementValue = 1) { return { data: () => ({ count: 0 }), methods: { increment() { this.count += incrementValue } }, render() { return h(WrappedComponent, { count: this.count, increment: this.increment }); } }; }Best Practices
- Use Vue 3's Composition API for complex HOCs to maintain reactivity
- Prefer props/default slots for content projection over direct template manipulation
- Apply HOCs at component declaration level to prevent unnecessary re-renders
- Combine with dependency injection for deep hierarchy support when needed
This pattern proves particularly valuable when building component libraries or managing cross-cutting concerns like analytics tracking, permission gating, or data fetching layers. While Vue's Composition API offers alternative composition patterns, HOCs remain a powerful tool for creating declarative, template-based component enhancements.
Citations:
- 1: https://reintech.io/blog/working-with-higher-order-components-in-vuejs
- 2: https://dev.to/wearethreebears/what-are-higher-order-components-and-how-to-use-them-in-vue-3n8e
- 3: https://dev.to/tefoh/hoc-in-vue-3-higher-order-components-2boe
- 4: https://legacy.reactjs.org/docs/higher-order-components.html
- 5: https://blog.logrocket.com/build-better-higher-order-components-with-vue-3/
- 6: https://learn-vuejs.github.io/vue-patterns/patterns/
- 7: https://vuejs.org/guide/essentials/component-basics
Attention: Verify External HOC Implementation
The plugin’s transformation logic—with the replacement of
defineComponentbyWithDesignConfigPropsDefineComponentfrom@opentiny/vue-common—properly sets up the HOC mechanism. However, as this relies on the external package, please ensure that:
- Existence & Implementation: The
WithDesignConfigPropsDefineComponentfunction is correctly implemented in both the Vue 2 and Vue 3 adapter modules (as noted in the PR summary) so that it properly wraps and enhances components as expected by HOC patterns in Vue.- Functionality Check: Its behavior (i.e., supplying additional props, managing internal state, etc.) aligns with the intended higher-order component design principles and does not introduce unexpected side effects.
Make sure to verify these aspects in
@opentiny/vue-commonto confirm consistent behavior across Vue versions.packages/vue-common/src/adapter/vue3/hoc.tsx (5)
15-15: Ensure robust handling of reactive objects.The code correctly unwraps reactive objects by checking for a
.valueproperty, but might benefit from more defensive checks or a helper function for clarity, especially since this handles various reactive object types from Vue 3 (ref, computed, etc.).
1-4: LGTM: Clean import statements with appropriate dependencies.The import statements correctly bring in the necessary dependencies for the HOC implementation.
5-9: LGTM: Well-structured HOC implementation.The HOC pattern is correctly implemented, preserving the original component's properties while adding the design configuration integration functionality.
38-42:✅ Verification successful
Verify JSX rendering in Vue 3 environment.
The JSX syntax used here assumes the environment is properly configured for JSX with Vue 3. Ensure this works correctly across different build configurations and environments.
🏁 Script executed:
#!/bin/bash # Check for JSX/TSX configuration in the project echo "Checking for JSX/TSX configuration..." fd -e json -e js "tsconfig|babel" --exec cat {} | grep -E "jsx|tsx" # Check for other components using JSX syntax echo "Checking for other components using JSX syntax..." fd -e tsx . --exec grep -l "<.*>" {} | head -5Length of output: 998
JSX Configuration Verified for Vue 3 Environment
- The project’s tsconfig/babel files consistently set JSX to
"preserve", confirming correct TSX handling.- Similar TSX usage in other components (as seen in test files) supports that the overall JSX setup is robust.
- The
<BaseComponent {...mergedProps} ref={innerRef}>pattern in the HOC is aligned with the established practices across the codebase.No changes are required regarding JSX rendering in Vue 3.
20-32:❓ Verification inconclusive
Clever proxy implementation, but needs thorough testing.
The proxy implementation elegantly exposes the inner component's methods and properties. However, ensure it's thoroughly tested with different property access patterns and edge cases (undefined refs, non-existent properties, etc.).
🏁 Script executed:
#!/bin/bash # Search for existing tests for this HOC implementation echo "Searching for tests related to DesignConfigPropsHOC..." rg -i "DesignConfigPropsHOC.*test|test.*DesignConfigPropsHOC" --type ts --type tsx --type js --type jsx # Check if any components are already using this HOC echo "Checking for components using DesignConfigPropsHOC..." rg -i "DesignConfigPropsHOC" --type ts --type tsx --type js --type jsxLength of output: 468
Thoroughly test proxy edge cases in the HOC implementation.
The proxy pattern here is a clever solution for exposing inner component properties and methods. However, our automated search for tests was inconclusive (partly due to file type recognition issues). Please ensure you add or verify unit tests covering scenarios such as:
- Cases where
innerRef.valueisundefinedornull.- Accessing properties that don’t exist (confirming that the proxy returns
undefinedsafely).- Diverse property access patterns to avoid any unexpected side effects.
Manual verification of test coverage is recommended.
🏁 Script executed:
#!/bin/bash # Re-run tests related to DesignConfigPropsHOC using file glob patterns to include tsx files echo "Searching for tests related to DesignConfigPropsHOC..." rg -i "DesignConfigPropsHOC.*test|test.*DesignConfigPropsHOC" --glob "*.tsx" --glob "*.ts" --glob "*.js" --glob "*.jsx" echo "Checking for components using DesignConfigPropsHOC..." rg -i "DesignConfigPropsHOC" --glob "*.tsx" --glob "*.ts" --glob "*.js" --glob "*.jsx"Length of output: 1501
Action Required: Enhance Test Coverage for Proxy Implementation
The proxy pattern here is a clever solution for exposing inner component properties and methods, but our investigation shows that tests for this behavior (especially within the Vue3 adapter) might not fully cover edge cases. Please make sure to verify and, if necessary, add tests that address the following:
- Cases where
innerRef.valueisundefinedornull;- Access to non-existent properties, ensuring that the proxy safely returns
undefined;- Various property access patterns to confirm that no unexpected side effects occur;
- Consistent test coverage in both Vue2 and Vue3 implementations where the HOC is used.
Manual verification of test coverage is strongly recommended, as our automated search did not conclusively locate the necessary tests.
| const params = {} | ||
| if(props.tiny_experimetal_props_hoc) { | ||
| result = import.meta.glob('./${value}.vue', { eager: true, query: { withPropsHOC: true } }) | ||
| } else { | ||
| result = import.meta.glob('./${value}.vue', { eager: true, query: params }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Inconsistent query parameter handling between code branches
In the first branch with withPropsHOC: true, you specify the query parameter directly, but in the else branch, you're passing the empty params object. This inconsistency may lead to unexpected behavior.
const params = {}
if(props.tiny_experimetal_props_hoc) {
result = import.meta.glob('./${value}.vue', { eager: true, query: { withPropsHOC: true } })
} else {
- result = import.meta.glob('./${value}.vue', { eager: true, query: params })
+ result = import.meta.glob('./${value}.vue', { eager: true })
}📝 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.
| const params = {} | |
| if(props.tiny_experimetal_props_hoc) { | |
| result = import.meta.glob('./${value}.vue', { eager: true, query: { withPropsHOC: true } }) | |
| } else { | |
| result = import.meta.glob('./${value}.vue', { eager: true, query: params }) | |
| } | |
| const params = {} | |
| if(props.tiny_experimetal_props_hoc) { | |
| result = import.meta.glob('./${value}.vue', { eager: true, query: { withPropsHOC: true } }) | |
| } else { | |
| result = import.meta.glob('./${value}.vue', { eager: true }) | |
| } |
| import { describe, expect, it } from 'vitest' | ||
| import { transformVirtualTemplate, transformVirtualTemplateForBuild } from '../src/core/index' | ||
|
|
||
| describe('index', () => { | ||
| describe('transformVirtualTemplate', () => { | ||
| it('pc|mobile|mobile-first', () => { | ||
| expect( | ||
| transformVirtualTemplate("import template from 'virtual:template?pc|mobile|mobile-first'") | ||
| ).toMatchSnapshot() | ||
| }) | ||
|
|
||
| it('pc|mobile=pc|mobile-first', () => { | ||
| expect( | ||
| transformVirtualTemplate("import template from 'virtual:template?pc|mobile=pc|mobile-first'") | ||
| ).toMatchSnapshot() | ||
| }) | ||
|
|
||
| it('pc|mobile|mobile-first=pc', () => { | ||
| expect( | ||
| transformVirtualTemplate("import template from 'virtual:template?pc|mobile|mobile-first=pc'") | ||
| ).toMatchSnapshot() | ||
| }) | ||
| }) | ||
|
|
||
| describe('transformVirtualTemplateForBuild', () => { | ||
| it('pc|mobile|mobile-first', () => { | ||
| expect( | ||
| transformVirtualTemplateForBuild("import template from 'virtual:template?pc|mobile|mobile-first'") | ||
| ).toMatchSnapshot() | ||
| }) | ||
|
|
||
| it('pc|mobile=pc|mobile-first', () => { | ||
| expect( | ||
| transformVirtualTemplateForBuild("import template from 'virtual:template?pc|mobile=pc|mobile-first'") | ||
| ).toMatchSnapshot() | ||
| }) | ||
|
|
||
| it('pc|mobile|mobile-first=pc', () => { | ||
| expect( | ||
| transformVirtualTemplateForBuild("import template from 'virtual:template?pc|mobile|mobile-first=pc'") | ||
| ).toMatchSnapshot() | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tests are missing validation of the new tiny_experimetal_props_hoc functionality
The tests only verify that the transformation works for different template string formats, but don't validate the behavior with the tiny_experimetal_props_hoc property, which is a key part of the changes. Consider adding tests that specifically verify this new functionality.
Add tests that simulate both cases where props.tiny_experimetal_props_hoc is defined and undefined:
it('handles props.tiny_experimental_props_hoc correctly', () => {
const transformed = transformVirtualTemplate("import template from 'virtual:template?pc|mobile'");
// Extract the generated function and test it with different props
// This is a simplified approach; you might need to use a more complex parsing strategy
const funcStr = transformed.match(/const template = \([\s\S]+?\}\s*$/)[0];
// Create a test to verify behavior with tiny_experimental_props_hoc=true
expect(funcStr.includes('withPropsHOC: true')).toBe(true);
// Optionally, you could use Function constructor to evaluate the generated code
// But that would require more complex setup and might be better as an integration test
});
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?
为 vue3及 vue2 增加 WithDesignConfigPropsDefineComponent 方法
组件中引入 defineComponent 的地方改为 WithDesignConfigPropsDefineComponent
import { $props, $prefix, $setup, WithDesignConfigPropsDefineComponent as defineComponent } from '@opentiny/vue-common'
目前仅 Button 进行了调整，Vue2，Vue3中都达到了预期的效果，Button 组件Vue3版本单元测试也能正常通过
Issue Number: #2940
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
<config-provider>component for better button property configuration.Tests