-
Notifications
You must be signed in to change notification settings - Fork 2
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
[#88] Change Webpack to Vite #117
[#88] Change Webpack to Vite #117
Conversation
efead03
to
e9e1d74
Compare
e9e1d74
to
12e8579
Compare
957d319
to
69bc71f
Compare
7de1f68
to
b02c927
Compare
b02c927
to
abadc8a
Compare
efb6d69
to
40f2fff
Compare
2770a85
to
9f2209a
Compare
86ed083
to
1742bce
Compare
9162189
to
f17b325
Compare
replaceLine( | ||
indexScssPath, | ||
`import 'assets/stylesheets/application.scss';`, | ||
`import 'assets/stylesheets/application.css';`, | ||
); | ||
replaceLine(indexScssPath, `import '@/dummy.scss';`, `import '@/dummy.css';`); | ||
|
||
// When using Vite |
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.
Maybe we should be doing it under if condition? I understand that it will not replace the content for the opposite case. Does this plugin have information on what we are using, vite or webpack?
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.
8999c21 ✅ ;-)
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 it is a wrong commit :) And I don't see fixes for it in other commits. I don't insist on it, just raised a question to consider whether it will be better or not.
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.
ca7ea8c ✅ 🙇
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.
Can we move the vite-template
folder into packages
just like cra-template
? 🤔
branch: string; | ||
}; | ||
|
||
const downloadTemplateRepository = ( |
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.
Should we move these functions to a helper? Then we can re-use them later as well. Just need to pass an option object, and get the expected folder
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.
2efd4b5 ✅
Please help me to review if that fits what you had in mind. I don't really like the dependence with the run-command (and so the need for the dest
argument)... but I think it is hard to do it differently 🤔
@hoangmirs if we move the Chore created in: |
272e58f
to
2efd4b5
Compare
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 we might need to do a POC to make sure we're okay to move from CRA to Vite then we can update our template structure 🤔
What happened 👀
Vite
and copy it in thevite-template
foldergithub
, rename%APP_NAME%
with the provided application name in some files, ...Insight 📝
@/
to map a resource inside thesrc
folderindex.scss
filevite-template
folder #120Proof Of Work 📹
To test with local changes in either the ./packages/cra-template or the ./vite-temaplte/ folders, use the following commands (this is documented in the packages/cli-tool/cli README.md file)
For Vite:
For Create React App (Webpack):
I can create a WebPack (CRA) app
I can create a Vite app
Tests on a new project are passing ✅