-
-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: resolver for unplugin-vue-components #86
Conversation
externals: [ | ||
...Object.keys(dependencies), | ||
], | ||
}, | ||
|
||
{ | ||
name: 'Unplugin-vue-component Resolver', | ||
entries: ['./src/resolver/index'], | ||
outDir: '../motion/dist', | ||
clean: false, | ||
declaration: 'node16', | ||
externals: [ | ||
'unplugin-vue-components', | ||
], | ||
rollup: { | ||
emitCJS: true, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplication and Maintainability Issue
There is significant duplication in the configuration settings for outDir
, clean
, declaration
, and rollup
across different modules (lines 8-13 and 22-30). This could lead to maintenance challenges and inconsistencies.
Recommendation: Consider abstracting common configuration settings into a shared variable or function. This would reduce duplication and make the codebase easier to manage and update. For example:
const commonSettings = {
outDir: '../motion/dist',
clean: false,
declaration: 'node16',
rollup: { emitCJS: true }
};
export default defineBuildConfig([
{ ...commonSettings, name: 'Nuxt module', entries: ['./src/nuxt/index.ts'], externals: [...Object.keys(dependencies)] },
{ ...commonSettings, name: 'Unplugin-vue-component Resolver', entries: ['./src/resolver/index'], externals: ['unplugin-vue-components'] }
]);
This approach not only reduces duplication but also centralizes control over these settings, making it easier to apply changes across all modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that for only 2 configs it will be okay; and it won't be changed often.
from: 'motion-v', | ||
} | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resolve
method implicitly returns undefined
when a component name is not found in the components
set. This implicit behavior can lead to potential issues if the consumer of this function does not check for undefined
values properly.
Recommendation: To make the code more robust and clear, explicitly handle the case where the component is not found by returning null
or a specific error object. For example:
if (!components.has(name)) {
return null; // or return { error: 'Component not found' };
}
hanks for the PR! |
Add a simple resolver for https://github.com/unplugin/unplugin-vue-components, based on reka-ui (https://github.com/unovue/reka-ui/blob/v2/packages/plugins/src/resolver/index.ts).