-
Notifications
You must be signed in to change notification settings - Fork 157
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
[#2176] Move from Vue CLI to Vite #2178
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.
LGTM!
@@ -19,6 +19,8 @@ node_modules | |||
/frontend/public/ | |||
!/frontend/public/favicon.ico | |||
!/frontend/public/index.html | |||
/frontend/.eslintcache |
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.
New files generated when linting.
frontend/src/main.ts
Outdated
@@ -4,6 +4,7 @@ import { FontAwesomeIcon } from '@fortawesome/vue-fontawesome'; | |||
import { ObserveVisibility } from 'vue-observe-visibility'; | |||
import hljs from 'highlight.js'; | |||
import router from './router/index'; | |||
import './styles/highlight-js-style.css'; |
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.
Required as the move to Vite somehow broke the existing highlighting mechanism.
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.
Seems like the code syntax highlighting is working for me even without this import statement. Could you please check again?
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.
File added to linting as it was imported into main.ts
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.
LGTM, this seems to work locally, and all test cases are passing.
Regarding browser support, it might be good if we could document it somewhere that we support most mobile and desktop evergreen browsers, as they have near full support for all ES6 features (Mainly just looks like Internet Explorer 11 that isn't supported). Perhaps we can add this in either the User Guide Appendix: Troubleshooting or a little footnote somewhere in the Developer guide for easy reference in the future.
frontend/package.json
Outdated
@@ -4,13 +4,13 @@ | |||
"description": "[![Build Status](https://travis-ci.org/reposense/RepoSense.svg?branch=master)](https://travis-ci.org/reposense/RepoSense) [![Build status](https://ci.appveyor.com/api/projects/status/gsbkj5qby3pjd6nw/branch/master?svg=true)](https://ci.appveyor.com/project/eugenepeh/reposense/branch/master)", | |||
"author": "", | |||
"scripts": { | |||
"serve": "vue-cli-service serve", | |||
"build": "vue-cli-service build", | |||
"serve": "vite preview", |
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 vite preview
command is used to locally preview the production build. It misses various essential functions such as hot reload, so it may not be the best option to choose for our serve
script. Let's use the simpler vite
command instead to start the dev server.
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.
Okay, I've updated the command!
I've also removed the import statement and reverted the CSS changes. I still have no idea what was causing it to break initially. Maybe it's the highlighting issue from #2184?
frontend/src/main.ts
Outdated
@@ -4,6 +4,7 @@ import { FontAwesomeIcon } from '@fortawesome/vue-fontawesome'; | |||
import { ObserveVisibility } from 'vue-observe-visibility'; | |||
import hljs from 'highlight.js'; | |||
import router from './router/index'; | |||
import './styles/highlight-js-style.css'; |
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.
Seems like the code syntax highlighting is working for me even without this import statement. Could you please check again?
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.
LGTM, thanks @sopa301 for suggesting and handling this migration to Vite! It's a great move that should boost performance and streamline our configuration.
Since it's a significant architectural shift, let's try to get one more approval before merging.
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.
LGTM, great job!
The following links are for previewing this pull request:
|
Fixes #2176
Proposed commit message
Other information
A few things worthy of discussion:
html
file from the config file. However, now that thehtml
file has been moved out of thepublic
folder (by design of Vite), this may not be a big concern. We can use a.env
file if we want to be able to edit the title and root folder from another file."chromeWebSecurity": false
into the Cypress config file. Since this is not very extensible to other browsers, we should also find another way to get the tests to work.