- 
                Notifications
    You must be signed in to change notification settings 
- Fork 342
chore: Migrated Vite to Rollup, Removed Vite related dependencies #6423
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
base: main
Are you sure you want to change the base?
Conversation
| 
 | 
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.
Thanks for handling this @yuhengshs! Have some questions on the configuration changes and there's some cleanup that can be addressed before merging. Curious how we are validating the compatibility of the new build artifacts against the existing artifacts?
07413ea    to
    3763460      
    Compare
  
    …cessive unnecessary sutff
3763460    to
    029097a      
    Compare
  
    9e4236d    to
    6b2a8ad      
    Compare
  
    15ff344    to
    89d3db4      
    Compare
  
    850860a    to
    2b64ed6      
    Compare
  
    293e1ff    to
    933afee      
    Compare
  
    | "vue": "^3.0.5", | ||
| "vue": "^3.5.14", | ||
| "vue-router": "4" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/node": "18.8.0", | ||
| "@vue/compiler-sfc": "^3.0.5", | ||
| "@vue/compiler-sfc": "^3.5.14", | 
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.
Remove these changes, as a reviewer it makes me question whether the changes impact the build artifacts
| import AmplifyUIVue from '@aws-amplify/ui-vue'; | ||
| import '@aws-amplify/ui-vue/styles.css'; | ||
|  | ||
| createApp(App).use(router).mount('#app'); | ||
| // Create app and register plugins | ||
| const app = createApp(App); | ||
| app.use(router); | ||
| app.use(AmplifyUIVue); | ||
| app.mount('#app'); | 
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.
Remove these changes, as a reviewer it makes me question whether the changes impact the build artifacts
| // Export all primitive base components | ||
| export { default as BaseInput } from './primitives/base-input.vue'; | ||
| export { default as BaseLabel } from './primitives/base-label.vue'; | ||
| export { default as BaseSelect } from './primitives/base-select.vue'; | ||
| export { default as BaseWrapper } from './primitives/base-wrapper.vue'; | ||
| export { default as BaseAlert } from './primitives/base-alert.vue'; | ||
| export { default as BaseBox } from './primitives/base-box.vue'; | ||
| export { default as BaseFieldSet } from './primitives/base-field-set.vue'; | ||
| export { default as BaseFooter } from './primitives/base-footer.vue'; | ||
| export { default as BaseForm } from './primitives/base-form.vue'; | ||
| export { default as BaseHeading } from './primitives/base-heading.vue'; | ||
| export { default as BaseSpacer } from './primitives/base-spacer.vue'; | ||
| export { default as BaseText } from './primitives/base-text.vue'; | ||
| export { default as BaseTwoTabs } from './primitives/base-two-tabs.vue'; | ||
| export { default as BaseTwoTabItem } from './primitives/base-two-tab-item.vue'; | 
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.
Are these new public exports?
| app.component('BaseInput', BaseInput); | ||
| app.component('BaseLabel', BaseLabel); | ||
| app.component('BaseSelect', BaseSelect); | ||
| app.component('BaseWrapper', BaseWrapper); | ||
| app.component('BaseAlert', BaseAlert); | ||
| app.component('BaseBox', BaseBox); | ||
| app.component('BaseFieldSet', BaseFieldSet); | ||
| app.component('BaseFooter', BaseFooter); | ||
| app.component('BaseForm', BaseForm); | ||
| app.component('BaseHeading', BaseHeading); | ||
| app.component('BaseSpacer', BaseSpacer); | ||
| app.component('BaseText', BaseText); | ||
| app.component('BaseTwoTabs', BaseTwoTabs); | ||
| app.component('BaseTwoTabItem', BaseTwoTabItem); | 
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.
What changed that requires the registaration of these modules?
| :required="false" | ||
| :disabled="false" | 
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.
Remove these changes, as a reviewer it makes me question whether the changes impact the build artifacts
Description of changes
This PR serves the purpose of migrating from
VitetoRollupinvuepackageIssue #, if available
Description of how you validated changes
Checklist
yarn testpasses and tests are updated/addeddocs,e2e,examples, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.