-
Notifications
You must be signed in to change notification settings - Fork 334
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
Vite
toRollup
invue
packageIssue #, if available
Description of how you validated changes
Checklist
yarn test
passes 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.