joshwcomeau / guppy

🐠A friendly application manager and task runner for React.js
ISC License
3.27k stars 154 forks source link

Reinstall feature #318

Closed AWolf81 closed 5 years ago

AWolf81 commented 5 years ago

Related Issue:

278

Summary:

Things to discuss:

Todos

Screenshots/GIFs: Screenrecording of reinstall screenrecording_reinstall_27112018

Application menu item grafik

Install Dependencies button (if node_modules missing) grafik

codecov[bot] commented 5 years ago

Codecov Report

Merging #318 into master will increase coverage by 0.47%. The diff coverage is 39.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
+ Coverage   25.35%   25.82%   +0.47%     
==========================================
  Files         152      152              
  Lines        3613     3729     +116     
  Branches      388      397       +9     
==========================================
+ Hits          916      963      +47     
- Misses       2426     2489      +63     
- Partials      271      277       +6
Impacted Files Coverage Δ
src/constants.js 100% <ø> (ø) :arrow_up:
src/components/LoadingScreen/LoadingScreen.js 0% <0%> (ø) :arrow_up:
src/services/read-from-disk.service.js 5.66% <0%> (+0.1%) :arrow_up:
src/components/ApplicationMenu/ApplicationMenu.js 0% <0%> (ø) :arrow_up:
src/reducers/projects.reducer.js 57.81% <0%> (-3.86%) :arrow_down:
src/components/ProjectPage/ProjectPage.js 0% <0%> (ø) :arrow_up:
src/components/ProgressBar/ProgressBar.js 100% <100%> (ø) :arrow_up:
src/actions/index.js 94.57% <100%> (+0.55%) :arrow_up:
src/sagas/task.saga.js 63.84% <50%> (-0.73%) :arrow_down:
src/sagas/dependency.saga.js 75.43% <52.17%> (-13.14%) :arrow_down:
... and 3 more
melanieseltzer commented 5 years ago

So, I've been trying to investigate a really weird performance issue in this branch and I'm confused because I can't pinpoint what's causing it. I've checked out each commit one by one to see if it gets introduced somewhere, and it appears and disappears at random.

The top menu will not allow me to click on anything, I roll over something and it just closes the menu. Also there's some weird jumpiness going on with scrolling (page jumps up and down).

This also happens with the reinstall (no errors to console). It flickers and loading screen disappears, but reinstall does eventually work and dependencies reappear. But the flicker is strange.

I'm stumped. I can't tell if it's my machine or widespread, do you encounter any of this on your machine? :/ As far as I can tell it's not on master and just in this branch.

AWolf81 commented 5 years ago

I don't have that issue. Have you tried to remove node_modules folder from Guppy and reinstall everything. Really a weird issue. I'll check it on Ubuntu maybe there is a similar behaviour.

Tested with vm MacOs 10.14 & Ubuntu 16.4.03 (I'll setup a Ubuntu 18.x soon to have a more recent version) with-out any issues.

joshwcomeau commented 5 years ago

I tried running this locally and I share @melanieseltzer's issue: "Electron helper" consumes 115% of my CPU after running the reinstall task =(

I also noticed some funkiness with the application menu:

appmenu

This sometimes happens when an option key is stuck, or something... but that isn't the case now, and the application menu works fine in other applications, so I think somehow there's something about this change causing that :/

joshwcomeau commented 5 years ago

Ah-ha - when I open Guppy and open the Redux Devtools, I see "LOAD_DEPENDENCY_INFO_FROM_DISK" firing over and over. Looks like there's some recursive loop somewhere.

AWolf81 commented 5 years ago

@joshwcomeau Thanks for your review. The performance issue should be fixed. Please check if it's fixed. I rewrote the dependency.reducer and added the loading status per project there. Also I'm hiding all tasks if node_modules folder is missing as I think that's easier than disabling every button in the UI. What do you think? Is it OK? I think it's OK as this really an edge case and shouldn't happen often.

There are two points open:

AWolf81 commented 5 years ago

@joshwcomeau can you please have a look at this? Is the performance OK now?

I've added everything from your review and removed the speech bubble. There is a new issue for the bubble so we can add this later. I think we can mark that issue as low priority as I think it's not that important to add.

melanieseltzer commented 5 years ago

@joshwcomeau Is this still blocking? Perf issues seem resolved and UI messages are hidden for now until UI is worked on, I think we can merge...