Skip to content
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

Update to Vite #665

Merged
merged 33 commits into from
Apr 2, 2024
Merged

Update to Vite #665

merged 33 commits into from
Apr 2, 2024

Conversation

ElectricS01
Copy link

@ElectricS01 ElectricS01 commented Aug 5, 2023

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description

Updated GeoGuess to use Vite instead of the slow and outdated Vue CLI, this has taken build times from 40-50s down to about 1s.

Main changes

  • Remove all Vue CLI related dependencies
  • Switched from Jest to Vitest for unit testing
  • Update environment variables to use VITE_APP_ instead of VUE_APP_
  • Added file name extensions to component and js imports
  • Changed Vuex index.js to use static imports because require is deprecated

What's broken

  • I have only tested the dev server, the built server and netlify
  • There are still 2 unit tests that don't work with Vitest

How Has This Been Tested?

Vite has been tested in production, with a built dev server and netlify, some options might not be working.

  • Stryker testing is working
  • Unit testing is mostly working
  • Cypress testing is working

Sample site with Vite here

@netlify
Copy link

netlify bot commented Aug 5, 2023

👷 Deploy request for geoguess2 pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit b219a59

src/components/home/MapsContainer.vue Outdated Show resolved Hide resolved
Copy link
Member

@BilelJegham BilelJegham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

thanks a lot, this PR will be improve développer experience

package.json Outdated Show resolved Hide resolved
.prettierrc.json Outdated Show resolved Hide resolved
src/lang/index.js Outdated Show resolved Hide resolved
@ElectricS01
Copy link
Author

Stryker and Cypress are tested and running, I also merged the latest commits from GeoGuess/GeoGuess

@ElectricS01 ElectricS01 changed the title [Not Fully Tested] Update to Vite Update to Vite Aug 7, 2023
@ElectricS01 ElectricS01 marked this pull request as ready for review August 7, 2023 10:35
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Troplo
Copy link
Collaborator

Troplo commented Sep 25, 2023

Nevermind I think I figured it out, this appears to work?

await wrapper.setData({ search: 'Toulouse' });
await wrapper.vm.$nextTick();
expect(wrapper.vm.axios.get).toBeCalledWith('https://photon.komoot.io/api/?q=Toulouse');
await wrapper.vm.$nextTick();
expect(wrapper.exists('#search-input'));

Copy link
Member

@BilelJegham BilelJegham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good for my, just a small rework on vuex modules.

You will see my code suggestion

src/store/index.js Show resolved Hide resolved
stryker.conf.json Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Oct 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ElectricS01
Copy link
Author

Can we get this merged so we can work on Vue 3

Copy link
Member

@BilelJegham BilelJegham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard to check all files.
But I think it's good.

@BilelJegham
Copy link
Member

Some issues before merge, I'm making some adjustments :

  • rebase on master
  • Add coverage vitest for codeCov
  • eslint issue with vitest
  • netlify build

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Nov 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Attention: 214 lines in your changes are missing coverage. Please review.

Comparison is base (4a1b827) 52.12% compared to head (b219a59) 81.84%.
Report is 1 commits behind head on master.

Files Patch % Lines
src/pages/StreetView.vue 35.05% 61 Missing and 2 partials ⚠️
src/pages/MedalsPage.vue 42.30% 45 Missing ⚠️
src/store/modules/settings.store.js 25.00% 30 Missing ⚠️
src/lang/index.js 36.00% 15 Missing and 1 partial ⚠️
src/components/Maps.vue 70.73% 12 Missing ⚠️
...omponents/history/gameResult/HistoryTimeDetail.vue 88.00% 9 Missing ⚠️
src/models/GeoMap.js 40.00% 6 Missing ⚠️
src/router.js 45.45% 6 Missing ⚠️
src/store/modules/home.store.js 86.66% 6 Missing ⚠️
src/plugins/StreetViewService.js 72.22% 5 Missing ⚠️
... and 9 more
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #665       +/-   ##
===========================================
+ Coverage   52.12%   81.84%   +29.71%     
===========================================
  Files          53       54        +1     
  Lines         752    11223    +10471     
  Branches      209      324      +115     
===========================================
+ Hits          392     9185     +8793     
- Misses        328     1999     +1671     
- Partials       32       39        +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ElectricS01
Copy link
Author

eslint issue with vitest is now resolved

@ElectricS01
Copy link
Author

Running npm run build also works so I assume it will work on Netlify

@BilelJegham BilelJegham changed the base branch from master to next April 2, 2024 11:49
@BilelJegham BilelJegham merged commit 3c4c065 into GeoGuess:next Apr 2, 2024
10 checks passed
BilelJegham added a commit that referenced this pull request Apr 12, 2024
* Doesn't load past home page

* Loads slightly more on home page

* Loads a bit more, buttons don't work

* Working

* Refactor imports

* Fully working

* Removed Jest as it is not Vite compatible

* Update src/lang/index.js

Co-authored-by: Bilel Jegham <BilelJegham@users.noreply.github.com>

* Reverse trailing commas and version number

* Everything but Cypress is working

* Merge branch 'master' of https://github.com/GeoGuess/GeoGuess into GeoGuess-master

* Fix eslint

* Update package.json

Co-authored-by: Bilel Jegham <BilelJegham@users.noreply.github.com>

* Update package.json

Co-authored-by: Bilel Jegham <BilelJegham@users.noreply.github.com>

* Vitest not working yet

* Vitest working a bit more

* Patched jest-google-maps-mock

* Update vite.config.js

* Vitest almost working

* Fixed stryker

* Add @ imports back

* Forgot some

* Update CardRoomMap.spec.js

* Update /store/index.js

* Merge

* fix: package

* fix: lint

* fix: add coverage test

* Fix patch package failure

* Fix ESLint with Vitest

* Update snapshot?

---------

Co-authored-by: Bilel Jegham <BilelJegham@users.noreply.github.com>
Co-authored-by: BilelJegham <contact.bileljegham@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants