joshwcomeau / guppy

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

[WIP] Show fish spinner while project is getting deleted from disk #294

Closed melanieseltzer closed 5 years ago

melanieseltzer commented 5 years ago

Related Issue:

282

Summary:

Show a fish spinner loading screen when the project is getting deleted. This is almost ready to go.

When it comes time to delete from disk, it seems like something (maybe rimraf?) is causing things to freeze and the screen cannot be rendered in time before things move on.

I don't think it's an issue with the LoadingScreen component since you can render it fine if you set the initialState in app-status.reducer.js to true. And the reducer seems to work fine (I'm doing a console.log in App.js and it's updating true/false as expected).

Any ideas? :/

Screenshots/GIFs:

LoadingScreen component:

screen shot 2018-10-15 at 7 05 21 pm
codecov[bot] commented 5 years ago

Codecov Report

Merging #294 into master will increase coverage by 0.02%. The diff coverage is 25.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
+ Coverage   20.46%   20.48%   +0.02%     
==========================================
  Files         236      239       +3     
  Lines        3675     3705      +30     
  Branches      376      379       +3     
==========================================
+ Hits          752      759       +7     
- Misses       2651     2672      +21     
- Partials      272      274       +2
Impacted Files Coverage Ξ”
src/constants.js 100% <ΓΈ> (ΓΈ) :arrow_up:
src/components/App/App.js 0% <0%> (ΓΈ) :arrow_up:
src/components/LoadingScreen/LoadingScreen.js 0% <0%> (ΓΈ)
src/components/LoadingScreen/index.js 0% <0%> (ΓΈ)
src/actions/index.js 94.39% <100%> (+0.1%) :arrow_up:
src/sagas/delete-project.saga.js 78.78% <25%> (-17.37%) :arrow_down:
src/reducers/app-status.reducer.js 57.14% <57.14%> (ΓΈ)
melanieseltzer commented 5 years ago

I think the Spinner is not displayed because the Redux action is not dispatched

Hm but I think the action is getting dispatched, because I have a console log inside App.js (soon to be moved when I address your feedback) as follows:

const { selectedProjectId, showLoadingScreen } = this.props;
console.log(showLoadingScreen);

And when I hit delete, console is logging true as expected (just without the UI on screen) and then logging false as the delete finishes. Doesn't that indicate the action is successfully dispatched and reducer is handling it?

Haroenv commented 5 years ago

I think this might be fixed (not fast though) by not using sync but async rimraf. WDYT?

melanieseltzer commented 5 years ago

@Haroenv Yes that was the issue, @AWolf81's promise code works great :) Tweaked slightly to follow the pattern of using a function to wait (like waitForChildProcessToComplete in task.saga), what do you think?

AWolf81 commented 5 years ago

@melanieseltzer Looks great. Can you please add error handling? I'm getting the following console.log (see screenshot below) after adding the following to waitForAsyncRimraf:

export function waitForAsyncRimraf(projectPath: string): Promise<void> {
  return new Promise((resolve, reject) =>
    rimraf(path.join(projectPath, 'node_modules'), err => {
      if (err) {
        console.log(err);
        reject(new Error('failed-to-delete'));
        return;
      }
      resolve();
    })
  );
}

The console.log is optional and just for debugging. Please also add a messagebox for handling the reject and stop delete process. I think something like Can't delete project from disk. Stop any open tasks in that directory, check write protection and please try again later.

I need to check what's blocking the directory. Maybe I'll create a new project so I can test delete.

grafik

melanieseltzer commented 5 years ago

@AWolf81 Is this related to #279?

I think the ENOTEMPTY error is a Windows specific issue that seems ongoing

melanieseltzer commented 5 years ago

@AWolf81 Yeah those sound good, def add to that issue πŸ‘